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
Typography: Stabilize typography block supports within block processing #63401
base: trunk
Are you sure you want to change the base?
Typography: Stabilize typography block supports within block processing #63401
Changes from all commits
2da34ad
b1bc1a5
b020e1e
84e2c1c
d551044
c971178
9beba00
298cc5f
cea5c02
7062b05
89da04a
be76869
8536354
4c2c130
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.
What about filters that update
supports
and inject their entries, which could be experimental? What about plugins that expect experimentalsupport
entries and make decisions based on them?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 was raised earlier and I believe what prompted @andrewserong to ask for additional opinions on the best path forward.
In a nutshell, the scenario we flagged, requires the migration to occur after the JS filters have been applied. Andrew found however that the block support filters adding attributes might be cleaner, and a better example, if the migration occurred before.
To cover both bases we may need to either
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’s also possible that 3rd party plugins run some checks based on the experimental block support syntax so the more I think about it the more it feels you should keep both for some time to let devs adjust or even forever. Best it would be to verify with some plugins that reference these experimental names. The approach shared by @youknowriad in #63401 (review) might be also a good first step if you want to exercise the current ecosystem.
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 discussion here!
I think we definitely want to at least support the experimental syntax forever so that block plugins out in the wild continue to work without being updated.
In terms of support within the filters and in regards to keeping both, how might that look? I.e. would it be something like:
__experimentalFontFamily
is set, copy tofontFamily
but leave__experimentalFontFamily
intactfontFamily
is set, copy to__experimentalFontFamily
for back-compat?If we do the latter as well as the former, after the filter is run, we might need to check if the value has changed (i.e. if the filter mutated one or the other values) and then resync them 🤔
I might not get a chance to update this PR this week, but I think a next step for me could be to add some tests that simulate possible use cases for plugins augmenting or inspecting the values. That might help illuminate possible cases for us to consider, or at the very least, help us limit the potential scope for affecting plugins out in the wild.
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.
Would it be technically feasible to convert the config objects in block supports that have experimental flags into proxy objects and handle the backward compatibility through augmented setters and getters? Let me share a similar case in PHP to better illustrate the idea:
https://github.com/WordPress/wordpress-develop/blob/74e03e3cbef2f2565028f446c76acb2dabf749bd/src/wp-includes/class-wp-block-type.php#L353L387
That allows to always use in core the new names but correctly match properties through legacy name when necessary.
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.
One final aside.
I have no real data to support my assumption but based on feedback I got at WordCamps and from GitHub reports, the most common scenario is removing UI controls for these features. I don’t know how that translates into code here though.
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.
Removing features is definitely a common scenario. It might be good to know whether folks doing that are not impacted that much.
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.
Good points, I think at the very least this PR should handle the basic ways that plugins might disable controls in both JS and PHP. I think this leans me toward applying the transformation twice in JS (once before running the filter, once after), so that we can capture it. Example test code:
In
trunk
that'll remove the Letter Case option from the Paragraph block, but not with this PR applied. Ideally it'd recognise if the value has changed after the above filter is run, and migrate it accordingly.I'll have a play with it this week.
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 would be fantastic, if possible. We have a lot of "Editor curation" content out there that shows folks how to disable/enable block supports with JS and PHP. Either way, I think the developer community will be quite excited about this stabilization effort.
Even if the transformation was not applied twice, you could do something like this to support 6.7 while maintaining backward compatibility, right?
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.
Yes, that example should work nicely for plugins that wish to be forward and backward compatible.
For now, I think I've gotten it working fairly well by applying the transformation a second time for both the block supports and deprecations in 6934b8a. I've added a couple of tests to cover the scenario we've described here.
The implementation still assumes that plugins aren't watching for current values before overriding things, but at least this gives us coverage for plugins that are attempting to switch things off.
I'll give this PR a bit more of a manual test, though, as it seems right to me now, but I'm not sure if I'm thinking is right 😄
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 util is useful as it needs to run a few times. I see it only does any processing when they
typography
object is provided, so it should be fast enough.