Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve caching in wp_get_global_styles_svg_filters #3926

Closed
wants to merge 7 commits into from

Conversation

spacedmonkey
Copy link
Member

Trac ticket: https://core.trac.wordpress.org/ticket/57568


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spacedmonkey Almost LGTM, just one point of feedback.

src/wp-includes/global-styles-and-settings.php Outdated Show resolved Hide resolved
@spacedmonkey spacedmonkey requested review from felixarntz and oandregal and removed request for oandregal January 30, 2023 12:21
@spacedmonkey
Copy link
Member Author

Ready for review

@hellofromtonya
Copy link
Contributor

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

What about unit tests?

Copy link

@ajlende ajlende left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working for me—the caching matches what is done for the stylesheets which is good 👍

@hellofromtonya
Copy link
Contributor

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
Copy link
Member Author

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 :(

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spacedmonkey LGTM, except for 2 small nit-picks below.

*
* @covers ::wp_get_global_styles_svg_filters
*
* @ticket 56970
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @ticket 56970
* @ticket 57568

@@ -0,0 +1,38 @@
<?php

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary blank line.

Suggested change

@felixarntz
Copy link
Member

@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.

@felixarntz
Copy link
Member

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

@felixarntz felixarntz closed this Feb 1, 2023
@oandregal
Copy link
Member

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants