Make WordPress Core

Opened 4 years ago

Last modified 2 years ago

#49446 assigned enhancement

cache the results of get_uploaded_header_images()

Reported by: pbiron's profile pbiron Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch
Focuses: administration, performance Cc:

Description

If the current theme supports custom-header and is_random_header_image() is true, then _media_states() calls get_uploaded_header_images() for every row in the media list table.

If the results of get_uploaded_header_images() were cached, the media list table would render much quicker, especially if the number of media displayed per page is large.

There is already an inline comment @todo Caching which has apparently been there since get_uploaded_header_images() was added in [17757].

Attachments (2)

49446.diff (923 bytes) - added by SergeyBiryukov 4 years ago.
49446.2.diff (1.0 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (18)

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


4 years ago

@SergeyBiryukov
4 years ago

#2 in reply to: ↑ description ; follow-up: @SergeyBiryukov
4 years ago

Replying to pbiron:

If the current theme supports custom-header and is_random_header_image() is true, then _media_states() calls get_uploaded_header_images() for every row in the media list table.

Even with caching added, saving $header_images to a static variable in _media_states() seems like a quick win to avoid having to go through wp_list_pluck() for the result of get_uploaded_header_images() for every row.

#3 in reply to: ↑ 2 @pbiron
4 years ago

Replying to SergeyBiryukov:

Even with caching added, saving $header_images to a static variable in _media_states() seems like a quick win to avoid having to go through wp_list_pluck() for the result of get_uploaded_header_images() for every row.

Agreed...that's kind how I stumbled on this. I've got a custom plugin that does a relatively complex query hooked to display_media_states and storing the results in a static sped things up considerably.

I went looking for more improvements and was going to suggest exactly the patch you added...then found that inline comment about caching in get_uploaded_header_images() :-)

#4 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.5

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


3 years ago

#6 @SergeyBiryukov
3 years ago

49446.2.diff adds a missing @staticvar entry.

#7 follow-up: @pbiron
3 years ago

I wasn't aware of the @staticvar tag, so I went to PHP Documentation Standards handbook page and don't see it listed as one of the tags (altho a quick search thru the core source, I see ~100 uses).

The handbook page does have an entry for @static that states:

Note: This tag has been used in the past, but should no longer be used.
Just using the static keyword in your code is enough for PhpDocumentor on PHP5 to recognize static variables and methods, and PhpDocumentor will mark them as static

Should the handbook be updated to mention @staticvar?

p.s. is this ready to commit now?

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

#8 in reply to: ↑ 7 ; follow-up: @SergeyBiryukov
3 years ago

  • Keywords has-patch commit added

Replying to pbiron:

I wasn't aware of the @staticvar tag, so I went to PHP Documentation Standards handbook page and don't see it listed as one of the tags (altho a quick search thru the core source, I see ~100 uses).
...
Should the handbook be updated to mention @staticvar?

Yes, it's used in core but for some reason not mentioned in the handbook.

Note that it's different from the @static tag that was removed in #42803.

p.s. is this ready to commit now?

I think so :)

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


3 years ago

#10 in reply to: ↑ 8 @SergeyBiryukov
3 years ago

Replying to SergeyBiryukov:

Yes, it's used in core but for some reason not mentioned in the handbook.

It turned out the @staticvar tag should not be used :) Follow-up: #50426.

So 49446.diff would be the preferred patch here.

#11 @desrosj
3 years ago

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

#12 @whyisjake
3 years ago

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

In 48152:

Cache API: Cache the results of get_uploaded_header_images().

Fixes #49446.

Props pbiron, SergeyBiryukov, desrosj.

#13 @SergeyBiryukov
3 years ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

The commit message in [48152] is misleading, it provides a performance improvement for _media_states(), but does not actually add caching to get_uploaded_header_images().

I believe there's still benefit in doing the latter, as it's used in some other places.

#14 @SergeyBiryukov
3 years ago

  • Milestone changed from 5.5 to Future Release

#15 @SergeyBiryukov
3 years ago

  • Component changed from Cache API to Themes

#16 @desrosj
2 years ago

  • Owner desrosj deleted
  • Status changed from reopened to assigned

Not going to be able to work on this near term. Removing myself as owner so other interested contributors do not hesitate to take this on.

Note: See TracTickets for help on using tickets.