Deprecated query_string doesn't support array arguments

[nacin]

In [17999/trunk/wp-admin/includes/post.php] we now send along the post_status as an array.

However, if any plugin is hooking into query_string, then the array disappears, due to the is_scalar() check in WP::build_query_string() --

	function build_query_string() {
		$this->query_string = '';
		$this->query_string = http_build_query($this->query_vars);
		foreach ( (array) array_keys($this->query_vars) as $wpvar) {
			if ( '' != $this->query_vars[$wpvar] ) {
				$this->query_string .= (strlen($this->query_string) < 1) ? '' : '&';
				if ( !is_scalar($this->query_vars[$wpvar]) ) // Discard non-scalars.
					continue;
				$this->query_string .= $wpvar . '=' . rawurlencode($this->query_vars[$wpvar]);
			}
		}

		// query_string filter deprecated.  Use request filter instead.
		if ( has_filter('query_string') ) {  // Don't bother filtering and parsing if no plugins are hooked in.
			$this->query_string = apply_filters('query_string', $this->query_string);
			parse_str($this->query_string, $this->query_vars);
			
		}
	}

A solution would be to instead build $this->query_string with http_build_query(). However, we want RFC 1936, rather than 1738 (rawurlencode vs urlencode). In PHP 5.3.x-dev, http_build_query() can accept an enc_type, but that doesn't help us.

Our own _http_build_query() does have an encoding parameter, but it's just true/false. We could perhaps overload that -- possibly by leveraging the same constants as PHP does in 5.3.x -- to support raw url encoding.

This is marked for 3.1.4 since our array in 3.1.3 ended up getting unset whenever query_string was used, which then sets post_status to future/private/publish/draft but not inherit, thus zeroing out the attachments page.

For 3.1.4, I suggest we move from the array callback back to the comma-delimited string form. That would mean reverting [18047] (optionally) and doing an implode of $states in [17999/trunk/wp-admin/includes/post.php].

For 3.2, we need to look at the other solutions proposed.

For 3.3 I'd like to start reporting deprecated filters.

[markjaquith]

Couldn't we earmark the non-scalar values, and then re-insert them into $this->query_vars after the parse_str call, if not set by the filter?

[nacin]

See #17556. The fix for 3.1.4 was attached there. I'm dropping it in, leaving [18047] for now.

[nacin]

In [18053]:

Don't use array calling for post_status in wp_edit_attachments_query() to prevent any use of the deprecated query_string filter (as in, don't use it) from tanking the page. see #17556, #17559. for 3.1.

[nacin]

In [18054]:

Don't use array calling for post_status in wp_edit_attachments_query() to prevent any use of the deprecated query_string filter (as in, don't use it) from tanking the page. see #17556, #17559. for the 3.1 branch.

[nacin]

[18053], obviously, was applied to trunk, rather than 3.1.

[nacin]

Mark ran a search and found that less than 8 plugins in the directory even use the query_string filter. So we're thinking that for 3.3, it can be yanked.

The query string can be dynamically built with __set() or it can be statically built as before. With more and more arguments being array-style, the query string is more and more inconsistent, so I don't really care which solution is chosen.

[nacin]

And thus for 3.2, I'm wondering if we just close it as fixed on [18053]. With array-style being used already, this was a ticking time bomb.

Perhaps we can offer a _deprecated_filter() call now so we can feel better about removing it in 3.3.

[ryan]

We'll do a _deprecated_filter() call if we can get it done pre-RC. Otherwise close as fixed.

[nacin]

Attached patch implements _deprecated_hook(). Usage:

This function is to be used for every hook that is deprecated, when any callback is attacked to the hook, as determined by has_action() or has_filter(), and shall be called before the hook is fired.

Working on a modification to Log Deprecated Notices to make sure I can glean all the right information.

[scribu]

Small detail: wouldn't 'deprecated_hook_run' be a more consistent name for the action?

[nacin]

They're different:

• deprecated_file_included
• deprecated_function_run
• deprecated_argument_run

I screwed up with the last one, it should have been deprecated_argument_used.

I tried to come up with the best verb for it, given that they're only reported when used. Then again, since these functions would only be called when the hook is used (based on external use of has_action or has_filter), then it doesn't matter much. deprecated_hook_fired, _triggered, _used, _run... All the same to me.

New patch that adds a few more deprecated hooks, based on research done in #10441. Still need someone to track down version numbers for a number of hooks.

[westi]

I think the patch is too clever.

We should have wrappers for do_action and apply_filters which we call for deprecated actions/filters.

e.g. do_deprecated_action and do_deprecated_filter

Inside these functions we put all of the has_action and reporting logic and don't change the way the code works.

I think this is much cleaner than littering the code with has_action ... do_action fragments.

[scribu]

Besides that, in some places you're calling the deprecated hook within the if block, while in others it's called before it.

A do_deprecated_action() function would reduce duplicated code, but would also have to have a different signature, which could get messy. First try:

function do_deprecated_action( $old_action, $version_deprecated, $new_action = '', $rest_of_args = array() )

[nacin]

That was an approach also attempted in #10441. That was my first approach here, but didn't like it much.

I think there might be situations where the check won't be so straightforward. Think _deprecated_argument() -- we don't make an empty() check internally, because it's sometimes not always that simple. (We also potentially save a function call this way.)

I can try it, however.

[nacin]

You're right scribu, that's a mistake on my part. I'll handle it.

And yes, the function signature gets messy. Your approach is what I had, but I moved the array of arguments to the front.

[nacin]

Attached patch uses a do_deprecated_action() and apply_deprecated_filters() syntax. (Untested.)

Not sure I like it. There was an argument to be made for parsing, but as long as any parsing script always assumed that the very next line (or hook) after _deprecated_hook() was what was being deprecated, I think that's a moot point.

Thoughts?

[nacin]

Closing as fixed for 3.1.4 and 3.2. We'll do this in 3.3.

[scribu]

How about this:

In Core, we keep a list of deprecated hooks inside a function:

function wp_get_deprecated_hooks() {
  return array(
    array( 'query_string', '3.2' ),
    array( ...
  );
}

Then, from Log Deprecated Notices (or another plugin), we hook into 'all' and check that array etc.

[scribu]

Darn it, keep finding this ticket instead of #10441.