Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#46351 closed defect (bug) (fixed)

Properly re-fill the `$meta` param of deprecated `wpmu_new_blog` action

Reported by: davidbinda's profile david.binda Owned by: jeremyfelt's profile jeremyfelt
Milestone: 5.1.1 Priority: normal
Severity: normal Version: 5.1
Component: Networks and Sites Keywords: has-patch commit fixed-major
Focuses: multisite Cc:

Description

The updates in r43548 changed the way how $meta value passed to wpmu_new_blog is being populated.

It introduces the $site_data_whitelist array (see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/ms-functions.php?rev=43548#L1288 ) which contains keys of properties which are now being passed to wp_insert_site, where those are being processed, and removes those keys from the $meta array.

The list of keys was previously used for identifying what properties are being saved as site options, but those were still passed to wpmu_new_blog action in terms of $meta param. (see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/ms-functions.php?rev=42976#L1309 )

That said, after the 5.1.0, the $meta in wpmu_new_blog no longer contain any of following properies:

<?php
$site_data_whitelist = array( 'public', 'archived', 'mature', 'spam', 'deleted', 'lang_id' );

Further, in r43654 the WPLANG, previously default meta in wpmu_create_blog was moved elsewhere, and is also not available among $meta in wpmu_new_blog action.

While the wpmu_create_blog is no longer the recommend way of creating new blogs, and wpmu_new_blog action have been deprecated, there still are some integrations out there, which may rely on the previous version of the $meta array.

I'm attaching a patch for properly re-filling the values and a unit test demonstrating the issue. The patch is introducing more values to the $meta, since the current implementation really wants to re-populate the $meta variable in case there are some callbacks hooked to wpmu_new_blog action, so it's easier to pass more data (with default values), than adjusting the wp_insert_site function the way it only passes the same limited set of properties.

Attachments (3)

46351.diff (2.5 KB) - added by david.binda 6 years ago.
46351.2.diff (4.9 KB) - added by david.binda 6 years ago.
46351.3.diff (4.8 KB) - added by david.binda 6 years ago.

Download all attachments as: .zip

Change History (18)

@david.binda
6 years ago

#1 @jeremyfelt
6 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 5.1.1

Thanks, @davidbinda - this makes sense at first glance. I'm going to move to the 5.1.1 milestone.

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


6 years ago

#3 @pbiron
6 years ago

@davidbinda: I've just been testing your patch. Is your intention to exactly duplicate the behavior of WP <=5.0.3? If so, it does not do that. I don't think it does anything wrong, but is results in $meta containing more info than would be provided in 5.0.3.

For example, when adding a new site via /wp-admin/network/site-new.php, the value of $meta passed to functions hooked to wpmu_new_blog is:

in 5.0.3

<?php
$meta = array(
        'public' => 1,
        'WP_LANG' => get_network_option( $network_id, 'WPLANG' ),
)

in 5.1, without your patch

<?php
$meta = array(
        'WPLANG' => get_network_option( $network_id, 'WPLANG' ),
)
  • so, yes, without the patch the value of public is not passed

in 5.1, with your patch

<?php
$meta = array(
        'public' => 1, 
        'archived' => 0,
        'mature' => 0,
        'spam' => 0,
        'deleted' => 0,
        'lang_id' => 0,
        'WPLANG' => get_network_option( $network_id, 'WPLANG' ),
)
  • the values of archived, mature, spam, deleted and lang_id where not passed in 5.0.3

So, again, I'm not suggesting that your patch is wrong, just that it results in slightly different behavior (for the default case of creating a site in the WP admin module) than 5.0.3.

Last edited 6 years ago by pbiron (previous) (diff)

#4 @david.binda
6 years ago

Thanks for testing the patch!

So, again, I'm not suggesting that your patch is wrong, just that it results in slightly different behavior (for the default case of creating a site in the WP admin module) than 5.0.3.

Yup, I wanted to introduce just minimal changes to the existing code, and was accepting the slightly different behaviour as a good enough.

But, I've re-visited the code and the patch in order to achieve the same behaviour, and turns out the necessary changes are not that huge and do no make the code less readable. Please, see the updated patch.

Thanks again!

@david.binda
6 years ago

#5 @pbiron
6 years ago

@davidbinda: the revised patches looks good!

When adding a site interactively, I get the same array passed to my function hooked to wpmy_new_blog as I do in 5.0.3. When I "manually" call wpmu_create_blog() with additional $meta, that additional $meta also gets passed the same as it does in 5.0.3.

Something is screwy with my local setup and I can't run unit tests right now, so I can't test the unit test in your patch. However, visually inspecting it, in test_wpmu_new_blog_action_backward_compatible(), shouldn't

remove_action( 'populate_options', array( $this, 'wpmu_new_blog_callback' ), 10 );

be

remove_action( 'wpmu_new_blog', array( $this, 'wpmu_new_blog_callback' ), 10 );
Version 0, edited 6 years ago by pbiron (next)

@david.binda
6 years ago

#6 @david.binda
6 years ago

Thanks for the code review and the feedback!

You're right, a wrong action name was used in the remove_action call. However, inspecting the WP_UnitTestCase_Base::__restore_hooks showed that there is no need to remove action/filter set in one test, as the method "Restores the hook-related globals to their state at setUp() so that future tests aren't affected by hooks set during this last test."

I've adjusted the patch accordingly (removing the remove_action call altogether).

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


6 years ago

#8 @pbiron
6 years ago

@davidbinda thanx for the revised patch. Looks good to me!

#9 @lukecarbis
6 years ago

  • Keywords commit added

#10 @jeremyfelt
6 years ago

  • Owner set to jeremyfelt
  • Status changed from new to reviewing

#11 @jeremyfelt
6 years ago

This looks good, thanks @davidbinda and @pbiron!

Clarification for my own future sake:

The $meta that was passed to wpmu_create_blog() prior to 5.1.0 was "promoted" (for lack of a better word) to top level keys in $data when passed to wp_insert_site() in 5.1.0+.

The 5.1.0 back-compat handling for the wpmu_new_blog hook accounted for $meta, but only for data passed to the new options key (yet to be documented, see #45061).

#12 @jeremyfelt
6 years ago

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

In 44805:

Multisite: Ensure wpmu_new_blog hook receives expected data in $meta.

Restores public, archived, mature, spam, deleted, lang_id, and WPLANG to the $meta data passed to wpmu_new_blog. This hook was deprecated in 5.1.0, but code using it still relies on this data.

Props david.binda, pbiron.
Fixes #46351.

#13 @jeremyfelt
6 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#14 @jeremyfelt
6 years ago

In 44806:

Multisite: Fix code formatting errors from r44805

See #46351.

#15 @jeremyfelt
6 years ago

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

In 44807:

Multisite: Ensure wpmu_new_blog hook receives expected data in $meta.

Restores public, archived, mature, spam, deleted, lang_id, and WPLANG to the $meta data passed to wpmu_new_blog. This hook was deprecated in 5.1.0, but code using it still relies on this data.

Props davidbinda, pbiron.
Merges [44805] and [44806] to the 5.1 branch.
Fixes #46351.

Note: See TracTickets for help on using tickets.