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

Remove usage of static methods and properties in WP_Theme_JSON_Resolver. #3540

Draft
wants to merge 7 commits into
base: trunk
Choose a base branch
from

Conversation

spacedmonkey
Copy link
Member

Trac ticket:


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.

@oandregal
Copy link
Member

Hey Jonny, have you seen WordPress/gutenberg#45171?

I'm interested in moving some of those tasks forward, and I'm optimistic they'll have a big impact in terms of performance and code quality. See for example this prototype that reduces the total time of a request from 514.44ms to 343.71ms, a 33% performance increase. I like to take a more data-driven approach to changes than we did in the past. Measure how performance is actually affected with changes. During the 6.1 cycle, there were many changes that required attention during RC, which is far from ideal.

Also, some of the changes in this PR become unnecessary when we land the new public API methods proposed WordPress/gutenberg#45171 For example: WP_Theme_JSON_Resolver::theme_has_support() would be wp_theme_has_theme_json (already landed in Gutenberg), and WP_Theme_JSON_Resolver::get_theme_data()->get_patterns() would be wp_get_remote_patterns_from_theme.

Additionally, it'd be good to land these sort changes in Gutenberg first, so they can be tested widely in the plugin so bugs are caught earlier.

@spacedmonkey
Copy link
Member Author

oandregal. I am seeing similar numbers in my testing, 0.4940s trunk compared 0.3423s to the same here.

The difference with this PR, as it uses classes properties, it is far more testable and readable. I strongly believe that the path forward is to convert this code to use an object.

It also means in the gutenberg plugin, that the global can be overridded, for better prototyping.

Backwards compat is going to be an issue here, as even through this class is private, it is being used by plugins already.

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.

2 participants