Avoid stripping attributes via group block migration when no layout is specified #63837
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Bug
|
Size Change: -2 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Jul 23, 2024
Comment on lines
+112
to
+113
| layout?.inherit || | ||
| ( layout?.contentSize && layout?.type !== 'constrained' ), |
There was a problem hiding this comment.
This fix makes sense. The deprecation is eligible when only experimental layout values are present and not when layout values are missing.
Ref: #57544 (comment)
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
While playing with the code, I noticed that returning It's not a blocker for this fix, but it's probably worth investigating separately. Screencast
CleanShot.2024-07-23.at.11.35.08.mp4 |
Mamaduka
approved these changes
Jul 23, 2024
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #63820
What?
Fixes a group block deprecation/migration that's a little too far reaching, and is causing attribute values to be stripped from perfectly valid blocks.
Why?
When a deprecation is run, it uses the list of attributes in the deprecation to filter out extraneous attribute values on the incoming parsed block.
In this case, the group block has a deprecation and migration that can match valid group blocks because the
isEligiblefunction on the deprecation forces the deprecation to run wheneverlayoutis not defined. According to the group block's definition and layout support, it's perfectly valid not to setlayout, a default value will be assumed.The problem is that the group block has some newer attributes (
allowedBlocksandtemplateLock) not included in the deprecation that are being stripped by the deprecation because of the wayisEligiblematches so easily.How?
I noticed that when
isEligiblematches because of the! layoutclause, it then triggers this early return in the migration:It returns the unmodified attributes (and without
templateLockandallowedBlocksbecause of the way deprecations work). I couldn't see much point to this code, so this PR fixes the issue by deleting it.It does leave me wondering if there is a purpose to the code being removed that I'm not seeing. It might be that in the original PR it seemed harmless so it flew under the radar during code review and these future problems were quite hard to anticipate.
Testing Instructions
Expected: the block is in
contentOnlymode due to the attribute in the snippetScreenshots or screencast