Migrations are extremely error-prone, and don't scale well

[ockham]

Description

I've brought this up before in Slack IIRC, but this time around, I think I have a clearer idea of the problem, and I think it's worth discussing it, so I'm filing this issue.

Migrations are really hard to get right: they're hard to write, to review, and to test. I think that this is mostly because, as the docs state,

Deprecations do not operate as a chain of updates in the way other software data updates, like database migrations, do.

Consider the following case (found while reviewing #31910):

In the button block, we have two migrations that we'll have a closer look at:

1. migrateBorderRadius turns a "flat" {"borderRadius":25} (with no unit) attribute into a nested {"style":{"border":{"radius":"25px"}}}.
2. migrateFontFamily (introduced by Font family: switch from CSS Custom Property to classes #31910) turns a nested {"style":{"typography":{"fontFamily":"var:preset|font-family|cambria-georgia"}}} into a flat {"fontFamily":"cambria-georgia"}. This migration is also added to the Group, Image, Pullquote, and Search blocks, since they also have block support for the font family setting.

There's existing test coverage for migrateBorderRadius, and the PR adds coverage for migrateFontFamily. Both tests pass. So what's the problem?

The problem is that (per the state of #31910 as of this writing) for a block that has attributes in both the old borderRadius, and the old {"style":{"typography":{"fontFamily":"..."}}} format, one of them won't be migrated. The reason for this is that we have a deprecation for the old borderRadius, and one for the old font family format there, but none for both.

This can be proven by using the following test fixture

<!-- wp:button {"borderRadius":25,"style":{"typography":{"fontFamily":"var:preset|font-family|cambria-georgia"}}} -->
<div class="wp-block-button" style="font-family:var(--wp--preset--font-family--cambria-georgia)"><a class="wp-block-button__link" style="border-radius:25px">My button</a></div>
<!-- /wp:button -->

and running npm run fixtures:regenerate test/integration/full-content/full-content.test.js, which will produce

<!-- wp:button {"style":{"typography":{"fontFamily":"var:preset|font-family|cambria-georgia"},"border":{"radius":"25px"}}} -->
<div class="wp-block-button" style="font-family:var(--wp--preset--font-family--cambria-georgia)"><a class="wp-block-button__link" style="border-radius:25px">My button</a></div>
<!-- /wp:button -->

Note that the border radius has been migrated, but the font family hasn't.

Which brings us back to

Deprecations do not operate as a chain of updates in the way other software data updates, like database migrations, do.

This means that it turns out that it's not enough to simply add a new deprecation that runs the migrateFontFamily migration; instead, we also need to change existing deprecations, such as the one that runs migrateBorderRadius, to also run migrateFontFamily, like so:

Diff

diff --git a/packages/block-library/src/button/deprecated.js b/packages/block-library/src/button/deprecated.js
index 542c5286ba..b1ec560973 100644
--- a/packages/block-library/src/button/deprecated.js
+++ b/packages/block-library/src/button/deprecated.js
@@ -224,7 +224,7 @@ const deprecated = [
 				</div>
 			);
 		},
-		migrate: migrateBorderRadius,
+		migrate: compose( migrateFontFamily, migrateBorderRadius ),
 	},
 	{
 		supports: {
@@ -319,7 +319,7 @@ const deprecated = [
 				</div>
 			);
 		},
-		migrate: migrateBorderRadius,
+		migrate: compose( migrateFontFamily, migrateBorderRadius ),
 	},
 	{
 		supports: {
@@ -414,7 +414,7 @@ const deprecated = [
 				</div>
 			);
 		},
-		migrate: migrateBorderRadius,
+		migrate: compose( migrateFontFamily, migrateBorderRadius ),
 	},
 	{
 		supports: {
@@ -484,7 +484,7 @@ const deprecated = [
 				/>
 			);
 		},
-		migrate: migrateBorderRadius,
+		migrate: compose( migrateFontFamily, migrateBorderRadius ),
 	},
 	{
 		supports: {
@@ -537,6 +537,7 @@ const deprecated = [
 			!! attributes.customGradient,
 		migrate: compose(
 			migrateBorderRadius,
+			migrateFontFamily,
 			migrateCustomColorsAndGradients
 		),
 		save( { attributes } ) {
@@ -653,11 +654,13 @@ const deprecated = [
 					.trim();
 			}
 			return migrateBorderRadius(
-				migrateCustomColorsAndGradients( {
-					...attributes,
-					className: newClassName ? newClassName : undefined,
-					borderRadius: 0,
-				} )
+				migrateFontFamily(
+					migrateCustomColorsAndGradients( {
+						...attributes,
+						className: newClassName ? newClassName : undefined,
+						borderRadius: 0,
+					} )
+				)
 			);
 		},
 		save( { attributes } ) {

The Button block's deprecated.js file alone has six deprecations that currently call migrateBorderRadius, so I added migrateFontFamily to all of them. (That might be overkill, since some of those deprecations might predate the introduction of the nested font family attribute; to find out which, I'd have to go through them one by one.)

The block also has a number of other deprecations that run migrations such as migrateCustomColorsAndGradients, etc. So basically, we need to go through all of these and change the ones where the "original" attribute format might have contained the old font family attribute style to run migrateFontFamily as part of their migrations. And then we should probably go through all the other migrations we're invoking in that file's deprecations, and make sure that they're included in all the places where they're required.

And as we've seen earlier, our tests don't protect us from that problem: Since those migrations are orthogonal, and we have dedicated tests for each, we didn't notice that those deprecations yielded the wrong result when a block needed both migrations.

---

In conclusion, I think that the current concept of deprecations that are supposed to map from any block and attribute format to the current one becomes untenably hard to maintain and review when scaling to multiple orthogonal migrations; plus our tests cannot guarantee that they're working properly.

These issues might not be apparent in a development setting, where we're dealing with little test content that is "throwaway" anyway, and where we constantly update our Gutenberg versions, so it's almost guaranteed that all the latest migrations are constantly run, thus keeping our test content well-formed. However, in a more real-world scenario, where the GB version is updated less frequently (through plugin, or even only Core updates), it becomes rather likely that some migrations are missed, and blocks lose (some of) their customization, or -- worse -- break altogether. (It'd be great if we could get some numbers on how often either of those things happen after a GB update.)

I believe that most of these things could be fixed by adopting an actual "incremental" migration model, where each migration is only required to update blocks from their previous format. This would significantly decrease the cognitive load of figuring out which migration is needed where, and it would make testing each individual migration separately actually sufficient to guarantee that they also work when run more of them need to be run. However, it would be a ton of work to change existing deprecations to that effect 😬

Step-by-step reproduction instructions

This space intentionally left blank.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[ockham]

cc @WordPress/gutenberg-core

[ntsekouras]

Another thing we should discuss is a way to handle deprecations in dynamic blocks as well. Use cases for this has already risen like here, here and here and I believe more and more are going to appear with the introduction of many new dynamic blocks (Post blocks).

Another difficult problem - which might be related to dev design decisions/guidelines - is the increasing use of context properties in dynamic blocks (Query Loop, Navigation, etc..). If a dynamic block relies on some context property for rendering and this property changes in the parent block that provides the context, there is no way we can have access to this value as it will no longer be there..

[glendaviesnz]

I opened a related issue and draft PR last week, which doesn't answer the question of having to update each deprecation, but looks at making them easier to manage over time by at least splitting them out into separate files.

[kohheepeace]

Hi @glendaviesnz ,
Thank you for showing a clean way of handling block versioning. I have a question, could you give me some advice on versioning a fixture with multiple features, like the core/button block?

I've created a versioning diagram for the core/button block as shown below.

[What is unclear to me is...]

1. If there is an upgrade in the core/button block, is it not necessary to upgrade the fixture in other features as well?
2. As start from where? in the diagram indicates, we don't know where a particular feature started, so do we need to add migration scripts to all versions as @ockham indicates?
3. The fixture for a particular feature (core/button__border__radius etc) does not correspond one-to-one to the version of core/button fixture, so we can't test whether each features work correctly for all versions of core/button, right ?

---

I don't understand this deprecation mechanism correctly yet, so the question itself may be gibberish, but I would be glad if I can discuss this topic 👍

[glendaviesnz]

Sorry for my bad question and poor English. I just want to confirm the below question.

No need to apologise - this is a great question, and very clear.

Is it necessary to create a FEATURE fixture for each version of the core block or not?

My understanding is that you would only need to create a fixture for a feature if the structure changes with a new version. With the gallery caption that feature has remained in exactly the same format, with the same placement, so I don't think there is any need to have a specific fixture for caption from v4 - v6, it seems like that would add a lot of clutter without adding any value.

If we decided to wrap the figcaption in a new wrapper div and and add some additional classes to it, then that would probably be a point to add a fixture ... but I don't think we currently have any sort of naming standard for how we might deal with that. Maybe it would be good to link it to the last version of the block that had that version of the feature, eg. if the latest gallery block had a completely different caption we would add core__gallery-with-caption__deprecated-6.html?

[kohheepeace]

so I don't think there is any need to have a specific fixture for caption from v4 - v6, it seems like that would add a lot of clutter without adding any value.

=> Confirmed and agreed 👍 a lot of clutter without adding any value.

If we decided to wrap the figcaption in a new wrapper div and and add some additional classes to it, then that would probably be a point to add a fixture

=> Confirmed 👍

if the latest gallery block had a completely different caption we would add core__gallery-with-caption__deprecated-6.html?

=>Sorry, I don't understand why filename is core__gallery-with-caption__deprecated-6.html. Is this because one before the latest version of core/gallery is 6 ?

[glendaviesnz]

Yes, correct. We are up to v7 of the gallery block, so if we had changed the figcaption in that version then maybe it makes sense to indicate that it was different just prior to that change, ie. in v6 of the block, so core__gallery-with-caption__deprecated-6.html. Does that make sense? This is up for discussion of course as I don't believe there is any agreed naming standard for this particular aspect of deprecations.

[kohheepeace]

Yes, correct. We are up to v7 of the gallery block, so if we had changed the figcaption in that version then maybe it makes sense to indicate that it was different just prior to that change, ie. in v6 of the block, so core__gallery-with-caption__deprecated-6.html.

=> Not 100% agreed because this is not intuitive for me at first. But I understand why 6 👍

This is up for discussion of course as I don't believe there is any agreed naming standard for this particular aspect of deprecations.

=> Yes 👍 It maybe better core__gallery-with-caption__deprecated-at-v6.html in my opinion.

Anyway Really thanks for helping me understand deprecation. I'm now having confidence about using deprecation. 😁 💯

---

[For Others Future Ref]

I also mention WordPress/gutenberg-examples#174 to suggest adding example of custom block fixture test not only e2e test. @ryanwelcher.

There is a mention and link to block fixture README in block testing guide. But it is difficult to add block fixture test by just reading Readme.

I personally imitate the implementation and file structure of jetpack to add block fixture test to my custom blocks.

[glendaviesnz]

It maybe better core__gallery-with-caption__deprecated-at-v6.html in my opinion.

Yes, I agree, that naming makes it clearer.