-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Conversation
There was a problem hiding this 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.
Ready for review |
Good work @spacedmonkey. The changes are using the same code patterns in https://core.trac.wordpress.org/changeset/55148 ✅ What about unit tests? |
There was a problem hiding this 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 👍
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 |
Added a unit test. Sadly there are no unit tests for this function currently in core :( |
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @ticket 56970 | |
* @ticket 57568 |
@@ -0,0 +1,38 @@ | |||
<?php | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary blank line.
@spacedmonkey @hellofromtonya I tested this in WP Admin with I gave it a secondary check also looking through the code paths: With this change there is no risk of breaking So this should be good for |
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 :) |
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.