WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#42251 closed enhancement (fixed)

Cache not found values for sites and network lookups by ID

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

Description

Currently when running get_site( 12345 ) and no ID with that site exists, the result is not being cached. That means every subsequent lookup will still cause a DB query to be fired, which is unnecessary. The exact same applies to networks as well.

Let's make sure falsy lookups are still cached under the ID checked.

That also implies that upon creating a new site or network, its respective ID cache must be cleared to not longer receive the cached falsy result. This is already the case for sites anyway, for networks it's a bit more tricky because we don't have a real API for that. However, with a dev-note clarifying that, also given how "edge-case" multiple networks are, I think we're good making that change.

Attachments (4)

42251.diff (2.0 KB) - added by flixos90 4 years ago.
42251-tests.diff (3.0 KB) - added by mnelson4 4 years ago.
Adds phpunit tests related to 42251
42251.2.diff (5.1 KB) - added by nielsdeblaauw 3 years ago.
42251.3.diff (6.8 KB) - added by flixos90 2 years ago.

Download all attachments as: .zip

Change History (23)

@flixos90
4 years ago

#1 @flixos90
4 years ago

  • Keywords has-patch added; needs-patch removed

42251.diff caches the value -1 for falsy lookups of sites and networks by ID. It furthermore ensures that upon inserting a new network, the cache value for that ID is cleaned.

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


4 years ago

#3 @jeremyfelt
4 years ago

  • Milestone changed from Awaiting Review to 5.0

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


4 years ago

@mnelson4
4 years ago

Adds phpunit tests related to 42251

#5 @mnelson4
4 years ago

I also tested this by creating a new site via the normal WP admin; and using one of our site's custom site creation code, and they both worked fine.

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


4 years ago

#7 @flixos90
3 years ago

  • Milestone changed from 5.0 to 5.1

#9 @pento
3 years ago

  • Keywords has-unit-tests needs-refresh added; needs-unit-tests dev-feedback removed
  • Milestone changed from 5.1 to 5.2

42251-tests.diff needs refreshing to apply to trunk.

Let's do this early in the 5.2 cycle, changing site caches makes me nervous, I'd like to see it tested on WP.com as early in the cycle as possible.

#10 @nielsdeblaauw
3 years ago

  • Keywords needs-refresh removed

I've fixed the conflicts and added some spacing to the tests.

#11 @SergeyBiryukov
3 years ago

  • Keywords commit added

#12 @flixos90
3 years ago

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

Patch is looking good. Due to this being a potentially impactful change in multisite, let's merge this early in the 5.3 cycle (see https://core.trac.wordpress.org/ticket/42251#comment:9).

I'll punt once we have a 5.3 milestone.

#13 @flixos90
3 years ago

  • Milestone changed from 5.2 to 5.3

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


2 years ago

@flixos90
2 years ago

#15 @flixos90
2 years ago

42251.3.diff ensures that caches for a newly created site are refreshed as soon as possible after its addition to the database. The absence of that was previously causing one of the new tests to fail.

Other than that, the patch only includes minor code formatting fixes and more consistent naming of tests.

#16 @flixos90
2 years ago

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

In 45910:

Multisite: Improve performance by caching not found lookups for sites and networks.

With this change, the result of a site or network lookup by ID will be cached even if the ID does not exist. When a new site or network is created, the cache for the respective new ID is cleared.

Props mnelson4, nielsdeblaauw.
Fixes #42251.

#17 @johnjamesjacoby
2 years ago

I’m concerned that the -1 return value might get ran through absint() somewhere and become a 1 which would be really bad, because that is typically the ID for the main site in the main network.

I don’t have a better idea though, and I don't have any evidence that this is an actual problem.

0 or false maybe… since site ID 0 will never exist?

#18 @spacedmonkey
2 years ago

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