-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix: Issues on block level settings #47842
Conversation
@@ -1551,7 +1551,7 @@ protected static function compute_preset_classes( $settings, $selector, $origins | |||
* @param string $selector Original selector. | |||
* @return string Scoped selector. | |||
*/ | |||
protected static function scope_selector( $scope, $selector ) { | |||
public static function scope_selector( $scope, $selector ) { |
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 just noticed that we're making scope_selector
a public API.
@oandregal, we had a similar discussion at #46571 (comment). Do you think this would be a valuable method for third parties, worth maintaining the BC?
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.
The class is still private, so this shouldn't be considered a public API. We have other methods that are also public in this class, though unnecessarily too many, in my view. In #45171 I list some that we need a public API for.
My concern in #46571 (comment) was that we were promoting or expecting third-parties to use it. If we expect anything to be used by 3rd parties, we need to create a proper public API outside this private class.
In this particular case, I don't know that we expect 3rd parties to use it. If that's the case, I'd support extracting it to its own method as a utility used by block supports and this class. If we don't expect anyone else to use it, I feel less pressure to extract it (though it'd still be a good thing).
Does that help clarify my rationale?
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.
It does help. Thank you, @oandregal!
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.
Exactly the class is internal. This function is not helpful as hopefully, third-parties never deal with low-level selectors, but if it turns out to be useful, we can extract it in a function. This PR is just fixing a regression that caused a crash when the functionality was originally merged. The function was already public and inside an internal class.
What?
During the backport to the core I noticed some regressions on the block-level settings in Gutenberg:
Testing
I pasted the following code on the code editor, saved the post, and verified everything works well: