Fix cover block content position not migrating correctly from deprecated version #29542
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] Regression
talldan
added
the
Backport to WP 6.7 Beta/RC
|
Size Change: +3 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
gwwar
approved these changes
Mar 4, 2021
There was a problem hiding this comment.
Nice work! This tests well for me @talldan ✨
| Trunk | Branch |
|---|---|
 |
 |
(The position is retained in trunk due to the css class on the container).
Instead I think it'd be better to encourage devs to copy the block settings directly from the block.json to make sure nothing is missed. Another idea could be to version the block.json and import that into a deprecation.
It's pretty natural to expect folks to copy and paste, even when it's not the right thing to do.
From https://developer.wordpress.org/block-editor/developers/block-api/block-deprecation/ it sounds like the intention is for one deprecation to be able to handle all potential block version migrations. (A bit trickier to handle on a long lived block, instead of chained updates). Maybe this would be clearer if we only had one deprecation available rather than a list.
I'm a fan of versioning the block.json. We could even maybe make it easy to scaffold the data migration tests for every block.json version.
Deprecations do not operate as a chain of updates in the way other software data updates, like database migrations, do. At first glance, it is easy to think that each deprecation is going to make the required changes to the data and then hand this new form of the block onto the next deprecation to make its changes. What happens instead, is that each deprecation is passed the original saved content, and if its save method produces valid content the deprecation is used to parse the block attributes. If it has a migrate method it will also be run using the attributes parsed by the deprecation. The current block is updated with the migrated attributes and inner blocks before the current block’s save function is run to generate new valid content for the block. At this point the current block should now be in a valid state.
| "name": "core/cover", | ||
| "isValid": true, | ||
| "attributes": { | ||
| "url": "http://localhost:8888/wp-content/uploads/2021/02/percy.jpg", |
There was a problem hiding this comment.
Minor+Non-blocking: Other fixtures appear to be using a base64 data url
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ### Bug Fixes | |||
|
|
|||
| - Fix a regression where the Cover block migration would not work with a non-default contentPosition ([#29542](https://github.com/WordPress/gutenberg/pull/29542)) | |||
There was a problem hiding this comment.
General question for my own learning: are we supposed to add a changelog entry to most PR changes for related packages?
|
Hey Robert @noisysocks I believe this might be the last that I know of atleast to be backported at this stage to WP 5.7. RC 3 will be released on Friday. We are aiming to get this in. |
noisysocks
pushed a commit
that referenced
this pull request
Mar 5, 2021
…ted version (#29542) * Add missing deprecation attribute for content position * Add new fixture for cover contentPosition deprecation * Update changelog
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Mar 5, 2021
…ted version (#29542) * Add missing deprecation attribute for content position * Add new fixture for cover contentPosition deprecation * Update changelog
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.
Description
Fixes #28656
This turned out to be the same issue as #28796, that the
contentPositionattribute was missing from the deprecation.This is one of the reasons I don't like the approach used in this codebase where a list of attributes is spread across multiple deprecations—it really easily leads to this sort of problem. Developers seem to start adding a new deprecation by copying the previous one, which is the wrong thing to do.
Instead I think it'd be better to encourage devs to copy the block settings directly from the block.json to make sure nothing is missed. Another idea could be to version the block.json and import that into a deprecation.
How has this been tested?
Added a new fixture to test for this.
Manual testing:
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: