WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 2 weeks ago

Last modified 2 days ago

#40201 closed enhancement (fixed)

Use get_site in clean_blog_cache

Reported by: spacedmonkey Owned by: flixos90
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: has-patch, dev-feedback, has-unit-tests
Focuses: multisite Cc:

Description

The clean_blog_cache takes a generic object as a param. This is a little strange as other clean_*_cache functions either take a list of ids or an ID. Now that the WP_Site object is in core, we should make sure we use it in this function. If we pass the $blog param through the get_site function it would have the following benefits.

  • All objects passed to function will be cast to WP_Site object. This would mean that the object would always domain and path properties.
  • Ability to pass a site id, as get_site allows for id lookup.
  • Better error handling. This function doesn't handle not passing an object very well.

Attachments (8)

40201.patch (716 bytes) - added by spacedmonkey 7 months ago.
40201.2.patch (954 bytes) - added by spacedmonkey 7 months ago.
40201.3.patch (1018 bytes) - added by spacedmonkey 7 months ago.
40201.diff (2.3 KB) - added by flixos90 6 weeks ago.
40201.2.diff (2.4 KB) - added by spacedmonkey 3 weeks ago.
40201.3.diff (2.0 KB) - added by flixos90 2 weeks ago.
40201.original-tests.diff (4.1 KB) - added by flixos90 2 weeks ago.
Tests to verify original behavior
40201.4.diff (4.2 KB) - added by flixos90 2 weeks ago.

Download all attachments as: .zip

Change History (31)

#1 @spacedmonkey
7 months ago

  • Keywords needs-patch needs-unit-tests 2nd-opinion added

@spacedmonkey
7 months ago

#2 @spacedmonkey
7 months ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


7 months ago

#4 @johnjamesjacoby
7 months ago

This seems sound.

if( is_object( $blog ) needs a whitespace fix before it's 100% ready for core.

Should be: if ( is_object( $blog )

#5 @spacedmonkey
7 months ago

  • Keywords has-unit-tests dev-feedback added; needs-unit-tests 2nd-opinion removed

Updated the patch. Now does better type checking. Patch also includes tests.

#6 @flixos90
7 months ago

+1 for this enhancement.

I just reviewed the latest patch and noticed that it doesn't handle the case where a site has just been deleted (get_site() will return null when passing an ID to it then). To fix that, we can use the logic that is currently part of refresh_blog_details(). It could probably be removed from that function afterwards.
Also I don't think we need to check is_object( $blog ) || is_int( $blog ) - due to the documented parameter we can simply pass $blog through get_site(). If the result is empty and $blog is numeric, the logic I mentioned above can be used to create a fake object.

There's something else to consider though: Maybe we should hold off with that enhancement until the changes for get_blog_details() have been merged (see #40180 and #40228 for background). With that, and with removing the blog-id-cache lookup in get_blog_id_from_url() (which doesn't make sense to have in there anymore, now that WP_Site_Query results are cached anyway; there's no ticket for that yet though) we don't need to use any of those weird caches anymore that consist of domain and path. That means that the only part of clean_blog_cache() that needs a full object would be the action hook. Not sure if we can work around it completely, but we could definitely optimize the clean_blog_cache() function even more once those changes are in place.

#7 @flixos90
7 months ago

I opened #40362 and #40363 as small related enhancements for clean_blog_cache().

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


2 months ago

@flixos90
6 weeks ago

#11 @flixos90
6 weeks ago

  • Keywords needs-unit-tests added; has-unit-tests removed
  • Milestone changed from Awaiting Review to 4.9

40201.diff builds upon the previous patch with the goal to become a 1-to-1-replacement for refresh_blog_details(). The function should be able to also deal with an ID for a site that has just been deleted (like refresh_blog_details() handles it). The patch deprecates refresh_blog_details() and its filter of the same name. The filter has been moved into clean_blog_cache() so that it still fires for code that now calls that function instead of refresh_blog_details().

Note that this is rather experimental: I definitely think we should aim for clean_blog_cache() to become a 1-to-1-replacement. Whether we deprecate the old function and filter immediately is another question - I just did it in this patch, but it can be easily reversed. If the function is deprecated, we also need to replace all calls to it in core with clean_blog_cache().

Putting this into 4.9 to give it some more thought. It aligns well with approaches like #40364 and #40180 / #40228. Also adding back the needs-unit-tests keyword, since you probably forgot to upload the tests previously @spacedmonkey :)

This ticket was mentioned in Slack in #core-multisite by desrosj. View the logs.


4 weeks ago

#13 @flixos90
4 weeks ago

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

#14 @flixos90
3 weeks ago

I think we can very well do this change in 4.9, but I'd rather hold off with the deprecation part. We can even replace all core calls to refresh_blog_details() with calls to clean_blog_cache(), but let's discuss deprecation in a broader scope, together with get_blog_details() and update_blog_details() both of which will be able to be deprecated after #40228 and #40364. This would furthermore allow us to deprecate early in a cycle vs a week before Beta now.

@spacedmonkey
3 weeks ago

#15 @spacedmonkey
3 weeks ago

I updated the patch. 40201.2.diff

Refactor the logic a bit here.

  • Set default param to 0 on clean_blog_cache. This means you can call clean_blog_cache() and it will just clear the current site- which is useful.
  • Pass all input through get_site, which will cast, null, int, object and wp_site to wp_site. Making sure we are dealing with wp_site object going forward.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 weeks ago

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


2 weeks ago

@flixos90
2 weeks ago

#18 @flixos90
2 weeks ago

40201.3.diff makes the following changes:

  • Do not deprecate the refresh_blog_details() function, only the action.
  • Bail if an empty parameter is given to clean_blog_cache().
  • Bail if no site could be found in clean_blog_cache() and the parameter is not numeric.

@flixos90
2 weeks ago

Tests to verify original behavior

#19 @flixos90
2 weeks ago

In 41715:

Multisite: Add specific tests for clean_blog_cache() and refresh_blog_details().

See #40201.

@flixos90
2 weeks ago

#20 @flixos90
2 weeks ago

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

In 41716:

Multisite: Establish clean_blog_cache() as a replacement for refresh_blog_details().

Going forward, clean_blog_cache() is recommended to be used instead of refresh_blog_details(). It has been adjusted to match the functionality of the latter, with the exception that it always requires a site ID or object to be passed. The refresh_blog_details action has been deprecated in favor of the clean_site_cache action. The function itself is not formally deprecated at this point, but will likely be in the near future.

Props spacedmonkey.
Fixes #40201.

#21 @flixos90
2 weeks ago

  • Keywords has-unit-tests needs-dev-note added; needs-unit-tests removed

#22 @flixos90
2 weeks ago

In 41717:

Multisite: Replace calls to refresh_blog_details() with clean_blog_cache().

Fixes #42077. See #40201.

#23 @jeremyfelt
2 days ago

  • Keywords needs-dev-note removed
Note: See TracTickets for help on using tickets.