WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#17559 closed defect (bug) (fixed)

Deprecated query_string doesn't support array arguments

Reported by: nacin Owned by:
Milestone: 3.1.4 Priority: normal
Severity: critical Version:
Component: Query Keywords:
Focuses: Cc:

Description

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.

Attachments (3)

deprecated_hook.diff (9.3 KB) - added by nacin 4 years ago.
deprecated_hook.2.diff (11.8 KB) - added by nacin 4 years ago.
do_deprecated_action.diff (12.8 KB) - added by nacin 4 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 @markjaquith4 years ago

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?

comment:2 @nacin4 years ago

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

comment:3 @nacin4 years ago

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.

comment:11 @nacin4 years ago

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.

comment:12 @nacin4 years ago

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

comment:13 @nacin4 years ago

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.

comment:14 @nacin4 years ago

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.

comment:15 @ryan4 years ago

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

@nacin4 years ago

comment:16 @nacin4 years ago

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.

comment:17 @scribu4 years ago

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

comment:18 @nacin4 years ago

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.

@nacin4 years ago

comment:19 @westi4 years ago

  • Cc westi added

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.

comment:20 @scribu4 years ago

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() )

comment:21 @nacin4 years ago

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.

comment:22 @nacin4 years ago

You're right scribu, that's

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

Version 0, edited 4 years ago by nacin (next)

@nacin4 years ago

comment:23 @nacin4 years ago

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?

comment:24 @nacin4 years ago

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:25 @scribu4 years ago

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.

comment:26 @scribu4 years ago

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

Note: See TracTickets for help on using tickets.