WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 10 days ago

Last modified 6 days ago

#50961 closed defect (bug) (fixed)

Fix missing ref array in pre_get_users

Reported by: andy Owned by: adamsilverstein
Milestone: 5.6 Priority: normal
Severity: normal Version: 4.4
Component: Users Keywords: has-patch
Focuses: Cc:

Description

Code docs in class-wp-user-query.php mark this as passed by reference but the actual call uses do_action instead of do_action_ref_array.

Index: wp-includes/class-wp-user-query.php
===================================================================
--- wp-includes/class-wp-user-query.php	(revision 212073)
+++ wp-includes/class-wp-user-query.php	(working copy)
@@ -227,7 +227,7 @@
 		 * @param WP_User_Query $this The current WP_User_Query instance,
 		 *                            passed by reference.
 		 */
-		do_action( 'pre_get_users', $this );
+		do_action_ref_array( 'pre_get_users', array( &$this ) );
 
 		// Ensure that query vars are filled after 'pre_get_users'.
 		$qv =& $this->query_vars;

Attachments (1)

50961.diff (1.9 KB) - added by SergeyBiryukov 3 months ago.

Download all attachments as: .zip

Change History (21)

#1 @SergeyBiryukov
4 months ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 5.6

Just noting this also applies to the pre_get_terms hook.

#2 @spacedmonkey
4 months ago

@adamsilverstein

#3 @adamsilverstein
4 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

Thanks for the ping @spacedmonkey.

This ticket was mentioned in PR #478 on WordPress/wordpress-develop by adamsilverstein.


4 months ago

  • Keywords has-patch added; needs-patch removed

Trac ticket:

#5 follow-up: @adamsilverstein
4 months ago

  • Keywords needs-patch added; has-patch removed

@andy I noticed we have tests for this and also found extensive plugin use where the original $this is modified.

I believe this works because PHP passes objects by reference, however changing to do_action_ref_array would certainly be easier to read code-wise. I'll give that change a test to make sure tests still work as expected.

#7 in reply to: ↑ 5 @andy
3 months ago

  • Resolution set to invalid
  • Status changed from assigned to closed

Hi Adam! Thanks for taking me back to school. :)

I went back and revisited the reason for the patch. The issue was that I used &$query in the action function's signature and got a warning about getting a value when a reference was expected. In fact, the reference to the object was working. I just needed to remove the & to stop the warning.

#8 @SergeyBiryukov
3 months ago

  • Keywords has-patch added; needs-patch good-first-bug removed
  • Resolution invalid deleted
  • Status changed from closed to reopened

I believe the change should still be made, not for functional difference but just for consistency with the other actions:

  • do_action_ref_array( 'pre_get_comments', array( &$this ) );
  • do_action_ref_array( 'pre_get_networks', array( &$this ) );
  • do_action_ref_array( 'pre_get_posts', array( &$this ) );
  • do_action_ref_array( 'pre_get_sites', array( &$this ) );
  • do_action_ref_array( 'pre_user_query', array( &$this ) );

50961.diff updates both pre_get_terms and pre_get_users to match.

#9 @adamsilverstein
3 months ago

I believe the change should still be made, not for functional difference but just for consistency with the other actions

Good point. 50961.diff looks good to me, I'll run the tests in travis to confirm.

#10 follow-up: @hellofromTonya
5 weeks ago

Restarted failing Travis jobs. Passing now.

Note: PHP8 job fails. If merged, need to add this to php8 tasks.

#11 @adamsilverstein
4 weeks ago

@SergeyBiryukov - reviewing 50961.diff - does changing $this to array( &$this ) change anything in terms of filter consumers use? I guess if all the tests pass we are fine!

#13 @desrosj
2 weeks ago

  • Version changed from trunk to 4.4

Looks like the user query instance was introduced in [29363] (4.4), and the term query one in [37572] (4.6)

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


11 days ago

#15 @hellofromTonya
10 days ago

Notes from yesterday's scrub.

@adamsilverstein noted to this question: Any risks for it being merged this late into 5.6?

I don't think so, on the other hand this is a purely internal change, so we could easily punt to 5.7 (not a priority)

This ticket was mentioned in Slack in #core by sergey. View the logs.


10 days ago

#17 in reply to: ↑ 10 @SergeyBiryukov
10 days ago

Replying to hellofromTonya:

Note: PHP8 job fails. If merged, need to add this to php8 tasks.

Just noting the PHP 8 failure is unrelated to the patch:

Warning: Class "PHPUnit\Util\Getopt" not found in /var/www/tests/phpunit/includes/phpunit6/compat.php on line 18
Error: Looks like you're using PHPUnit 9.4.2. WordPress requires at least PHPUnit 5.4 and is currently only compatible with PHPUnit up to 7.x.
Please use the latest PHPUnit version from the 7.x branch.

It looks like a wrong PHPUnit version was used for some reason. That does not happen on WordPress trunk.

The patch should not have any functional difference, committing shortly.

#18 @SergeyBiryukov
10 days ago

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

In 49637:

Users: Use do_action_ref_array() for pre_get_users and pre_get_terms actions.

This brings some consistency with the other similar actions:

  • pre_get_comments
  • pre_get_networks
  • pre_get_posts
  • pre_get_sites
  • pre_user_query

Follow-up to [29363] and [37572].

Props andy, adamsilverstein, hellofromTonya, desrosj, SergeyBiryukov.
Fixes #50961.

Note: See TracTickets for help on using tickets.