Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Style engine: migrate functions, classes and tests #3199
Style engine: migrate functions, classes and tests #3199
Changes from 15 commits
11a9aac
8d87cd1
05b1903
d4fa390
5543539
f044bf4
73288d1
aa291de
cfcc425
4b2128e
5da7cf4
d0de610
3d2f156
26cd739
57d81bc
8e3266d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
While this is intended to be private, it's open not just for instantiation, but extension.
Some thoughts:
final
class to at least block extension, or do we extend/plan to extend this class?debug_backtrace
for the caller of the constructor is essentially the only way to "properly" lock it down though, so not suggesting that).Same applies to all instances in the PR.
@peterwilsoncc do you have any thoughts here?
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.
I'm not sure of the strategy right now.
I expect that there is no plan to extend. Since this comes from a Gutenberg package, we expect to version it.
Backwards compat will foil that I guess. 🤷
Happy to take any advice onboard!
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.
@azaozz Do you have any thoughts on this?
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.
@costdev thanks for the ping.
If this is intended to be private, "core use only", it will perhaps have to be locked in a closure like the current Webfonts code is. That will ensure it can be updated in the future without concerns about backwards compatibility.
If not locked, back-compat will have to be maintained, even if the class is
final
.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.
Thanks @azaozz! If it's not locked and back-compat has to be maintained, I don't think there would be any benefit in having
@access private
. Would you agree?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.
Thanks for the advice here.
I think the class in question (
WP_Style_Engine_CSS_Declarations
), and the following, other, new classes:WP_Style_Engine_CSS_Rule
WP_Style_Engine_CSS_Rules_Store
WP_Style_Engine_Processor
should be fairly stable. Maintaining backwards compatibility will be expected and, I hope, not overly tortuous.
I'll remove
@access private
from these.WP_Style_Engine
, on the other hand, is the primary integration point and provides the bulk of the functionality.I expect it will have to be updated more frequently, especially given that this is a new API.
In that case, do folks think it's safer to refactor
WP_Style_Engine
to something similar to what's happening in webfonts? https://github.com/ramonjd/wordpress-develop/blob/bb0cbe3f557d16714ca109a28c2877d178adc94f/src/wp-includes/script-loader.php#L3059-L3058cc @aristath and @andrewserong for thoughts
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.
I think even this one is pretty stable, and good for us to commit to maintaining for backwards compatibility. The protected methods can be added to or rearranged safely in future versions, but the public methods appear to be ones that have been pretty stable in Gutenberg lately, and I think would be good to commit to in the long-term. I'd lean toward keeping this PR as-is.