Make WordPress Core

Opened 8 years ago

Last modified 5 weeks ago

#40362 new defect (bug)

Remove `blog-id-cache` cache group

Reported by: flixos90's profile flixos90 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: has-patch 2nd-opinion has-unit-tests
Focuses: multisite Cc:

Description

The function get_blog_id_from_url() is the only function that uses the old blog-id-cache group. Since the introduction of WP_Site_Query it's basically redundant in there though, since site query results are cached anyway.

Therefore I think we should remove the cache read and write actions in that function. We can then also remove the cache group completely (from clean_blog_cache() and registration as a "global group").

Attachments (1)

40362.patch (4.5 KB) - added by spacedmonkey 8 years ago.

Download all attachments as: .zip

Change History (16)

@spacedmonkey
8 years ago

#1 @spacedmonkey
8 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

My patch removes uses of the group in core and in tests. I have removed the global group, as plugins may be using it for something.

I believe we should be getting rid of the all cache look ups that use domain / path. I believe most domain mapping solutions out there will break the cache invalidation.

#2 @jeremyfelt
8 years ago

  • Keywords good-first-bug removed

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


8 years ago

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


8 years ago

#5 follow-up: @flixos90
8 years ago

  • Keywords 2nd-opinion added; dev-feedback removed

Per bug-scrub discussion:

Maybe we should hold off removing that cache, although it's redundant. That is because WP_Site_Query caches are invalidated on every change to any site (via last_changed), while blog-id-cache keys are only invalidated on updates to the specific site. So the blog-id-cache keys are much more "stable".

I'm not sure whether this is actually a reason to hold off, but we need to discuss and further investigate.

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


8 years ago

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


8 years ago

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


6 weeks ago

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


5 weeks ago

This ticket was mentioned in PR #8908 on WordPress/wordpress-develop by @Heiko_Mamerow.


5 weeks ago
#10

  • Keywords has-unit-tests added

#11 @heikomamerow
5 weeks ago

@soean and me did a PR... see above

#12 @johnjamesjacoby
5 weeks ago

This looks great. Nice job!

#13 in reply to: ↑ 5 ; follow-up: @johnjamesjacoby
5 weeks ago

Replying to flixos90:

Per bug-scrub discussion:

Maybe we should hold off removing that cache, although it's redundant. That is because WP_Site_Query caches are invalidated on every change to any site (via last_changed), while blog-id-cache keys are only invalidated on updates to the specific site. So the blog-id-cache keys are much more "stable".

I'm not sure whether this is actually a reason to hold off, but we need to discuss and further investigate.

Link to original discussion: https://wordpress.slack.com/archives/C03BVB47S/p1491848375654869

last_changed cache invalidation is still an issue.

Merging this PR will cause increased churn, specifically because of wpmu_update_blogs_date(), by way of _update_blog_date_on_post_publish() and _update_blog_date_on_post_delete().

#14 in reply to: ↑ 13 ; follow-up: @heikomamerow
5 weeks ago

Ok, see the point. Should we close the ticket then?

Replying to johnjamesjacoby:

Replying to flixos90:

Per bug-scrub discussion:

Maybe we should hold off removing that cache, although it's redundant. That is because WP_Site_Query caches are invalidated on every change to any site (via last_changed), while blog-id-cache keys are only invalidated on updates to the specific site. So the blog-id-cache keys are much more "stable".

I'm not sure whether this is actually a reason to hold off, but we need to discuss and further investigate.

Link to original discussion: https://wordpress.slack.com/archives/C03BVB47S/p1491848375654869

last_changed cache invalidation is still an issue.

Merging this PR will cause increased churn, specifically because of wpmu_update_blogs_date(), by way of _update_blog_date_on_post_publish() and _update_blog_date_on_post_delete().

#15 in reply to: ↑ 14 @johnjamesjacoby
5 weeks ago

Replying to heikomamerow:

Ok, see the point. Should we close the ticket then?

I reached out to Barry at WordPress.com to see what his thoughts are (and if our hunch is correct or not).

Let's leave this open until we hear back?

Note: See TracTickets for help on using tickets.