Make WordPress Core

Opened 16 months ago

Closed 16 months ago

Last modified 15 months ago

#57568 closed enhancement (fixed)

Improve caching in `wp_get_global_styles_svg_filters`

Reported by: spacedmonkey's profile spacedmonkey Owned by: flixos90's profile flixos90
Milestone: 6.2 Priority: normal
Severity: normal Version: 5.9.1
Component: Editor Keywords: has-patch
Focuses: performance Cc:

Description

Follow on from #56910 ( and [55148])

From #56910

Originally this ticket also covered the wp_get_global_styles_svg_filters function which is subject to a similar change, but since that is a separate effort it makes sense to break it out in a separate ticket.

Change the wp_get_global_styles_svg_filters function to use object caching over transients. This brings the code inline with wp_get_global_stylesheet.

Change History (19)

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


16 months ago
#1

  • Keywords has-patch added

#2 @spacedmonkey
16 months ago

@flixos90 @oandregal Can you take a look at #3926

#3 @oandregal
16 months ago

My recommendation would be to ship this in Gutenberg first, like any other change. Merge back in core when tested with real users. I see you started at https://github.com/WordPress/gutenberg/pull/47460

#4 @spacedmonkey
16 months ago

@oandregal If you look at that PR, was decided to move this, back to core.

#5 @flixos90
16 months ago

@oandregal Generally I would agree with you, however I think this change is of rather trivial nature after the other similar changes we have already established to be functional.

WordPress core releases are far less frequent than Gutenberg releases, and if we have to wait for this to first be implemented in Gutenberg and then released and backported, we would probably miss the 6.2 release.

Given the other similar change [55148] included, I think for this one it would be reasonable to include in 6.2 and then backport to Gutenberg (or even work on both at a similar time).

@spacedmonkey I see the current milestone of this ticket is "Future Release" though, so in that case it would be pointless to do it in WordPress first IMO. Will change it to 6.2 now, but let's keep discussing. If we punt it to a "Future Release", we might as well do it in Gutenberg first.

FWIW, the pull request is almost ready from my perspective, so technically nothing is preventing this from landing in 6.2.

Last edited 16 months ago by flixos90 (previous) (diff)

#6 @flixos90
16 months ago

  • Milestone changed from Future Release to 6.2

#7 @flixos90
16 months ago

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

#8 follow-up: @oandregal
16 months ago

@flixos90 Fair enough. While I'm not super stoked of shortcircuiting the process just because we can, I do see this is an edge case (because the function has been just removed from Gutenberg). Let's try this one in core first.

@spacedmonkey can you provide some manual testing instructions to make sure we cover all the cases?

#9 in reply to: ↑ 8 @spacedmonkey
16 months ago

Replying to oandregal:

@flixos90 Fair enough. While I'm not super stoked of shortcircuiting the process just because we can, I do see this is an edge case (because the function has been just removed from Gutenberg). Let's try this one in core first.

This is an edge case.

@spacedmonkey can you provide some manual testing instructions to make sure we cover all the cases?

This is just an extension of #56910 for constituency. There is no functional change here, so not much to test. Run some profiles if you got that.

@spacedmonkey commented on PR #3926:


16 months ago
#10

Ready for review

@hellofromTonya commented on PR #3926:


16 months ago
#11

Good work @spacedmonkey. The changes are using the same code patterns in https://core.trac.wordpress.org/changeset/55148

What about unit tests?

@hellofromTonya commented on PR #3926:


16 months ago
#12

Note: This PR will need to be tested in the WP Admin area per the testing instructions here to ensure there is no fatal error from not having wp_cache_*() functions available when load-styles.php runs.

@spacedmonkey commented on PR #3926:


16 months ago
#13

Good work @spacedmonkey. The changes are using the same code patterns in https://core.trac.wordpress.org/changeset/55148 white_check_mark

What about unit tests?

Added a unit test. Sadly there are no unit tests for this function currently in core :(

@flixos90 commented on PR #3926:


16 months ago
#14

@spacedmonkey @hellofromtonya I tested this in WP Admin with CONCATENATE_SCRIPTS enabled, load-styles.php is working fine as expected.

I gave it a secondary check also looking through the code paths: With this change there is no risk of breaking load-styles.php, since the function is only called in wp_global_styles_render_svg_filters(), and that function is only called in 2 specific places unrelated to load-styles.php.

So this should be good for commit IMO.

#15 @flixos90
16 months ago

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

In 55185:

Editor: Use a non-persistent object cache instead of transient in wp_get_global_stylesheet().

This changeset is part of a greater effort to enhance the caching strategy for theme.json based data. Similar to [55138], [55148], and [55155], the cache is currently ignored when WP_DEBUG is on to avoid interrupting the theme developer's workflow.

Props spacedmonkey, oandregal, flixos90, ajlende, hellofromtonya.
Fixes #57568.

@flixos90 commented on PR #3926:


16 months ago
#16

Committed in https://core.trac.wordpress.org/changeset/55185. @oandregal This should be straightforward to backport to Gutenberg.

@oandregal commented on PR #3926:


16 months ago
#17

Committed in https://core.trac.wordpress.org/changeset/55185. @oandregal This should be straightforward to backport to Gutenberg.

My understanding is that we don't do this, otherwise, it should have been done in Gutenberg _first_, correct?

Also: if the change was to be backported to Gutenberg, I'd recommend the author of the core change to create the pull request in Gutenberg, otherwise we'd run into human bottlenecks fast :)

#18 @milana_cap
15 months ago

  • Keywords add-to-field-guide added

#19 @milana_cap
15 months ago

  • Keywords add-to-field-guide removed
Note: See TracTickets for help on using tickets.