Block sequential deprecations result in attribute data loss

Description

I'm having trouble with block deprecations being run sequentially on a dynamic block.

I have created the following example: a block with two initial attributes (name and surname), which get renamed one after the other (to firstName and lastName, respectively) in two separate deprecation functions. What happens is that the second one being run, overrides the outcome of the first, resulting in the first updated attribute being lost.

In our case, being the surname the first one being run in the deprecated.js exported array, only the name -> firstName attribute is correctly kept.

Here's a link to the repo where the issue is happening: https://github.com/andreg/BlockDeprecations

Specifically, here's a link to the deprecated.js file: https://github.com/andreg/BlockDeprecations/blob/main/src/deprecated.js

Step-by-step reproduction instructions

Compile the block
Create a new page with the following content: 
Refresh the page and check the block data wp.data.select( 'core/block-editor' ).getBlocks()[0].attributes

You'll see thatthe firstName attribute is present and with the correct value, while the lastName attribute is missing, infact it's still present the previous surname attribute.

Screenshots, screen recording, code snippet

No response

Environment info

WordPress 6.4

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

Thanks for taking the time to submit this issue @andreg 👍

If I understand the problem correctly, I believe there's a misunderstanding of the way deprecations work in Gutenberg. This is completely understandable as I found them very unintuitive and misunderstood them myself when first working with them.

The key point here is that deprecations are not sequential. The deprecation docs state:

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

The documentation goes into much further detail so I'd strongly recommend giving that a read, maybe even a few times 😅.

How this relates to your situation depends on how many old versions of your block there are.

If you have a single deprecated version of the block with the name and surname attributes, you'd need only a single deprecation that migrates both attributes to their current naming.

If the original block was updated multiple times e.g. first updating one attribute name only, then later the second attribute as well, then you'd need two deprecations. Each deprecation needs to migrate all the attributes all the way to the very latest version. Same goes for any markup changes etc.

Deprecations work by finding the first deprecation which has a save function that produces valid content. As that deprecation should migrate that version of the block all the way to the latest current version, there is no sequential progression from say v1 to v2.

This is why the deprecation array should be ordered in terms of the most recent version to oldest as that reflects the probability the appropriate deprecation is hit first. I believe the intent behind this approach is performance.

What happens is that the second one being run, overrides the outcome of the first, resulting in the first updated attribute being lost.

This explains the behaviour you're noticing. Your second deprecation is matched first and migrates only the surname attribute to lastName. So rather than the outcome of the first update being lost, it is never happening.

I hope that helps 🤞

It's a tricky topic so please let me know if this comment or the docs aren't clear. If it is, I think we can close this issue.

Hi @aaronrobertshaw,

thank you for taking the time to reply to my question. I'm not exactly sure I'm folllowing the reasoning behind migrations, the way they have been explained in the documentation.

I have modified the example provided (https://github.com/andreg/BlockDeprecations/blob/main/src/deprecated.js), by making each deprecation to migrate all the attributes all the way to the current version of the data, however, I'm not getting why the v1 migration is still run after v2, since the sequence should stop (all attributes are updated in the v2 migration callback). I've left a couple of console.log calls for that.

Hope you can shed a bit more light on that!

Thank you!

I'm not exactly sure I'm folllowing the reasoning behind migrations, the way they have been explained in the documentation.

If you have any ideas or suggestions on how to improve the documentation, that sounds like a great follow-up. I get the feeling a visual diagram in addition to the textual explanation might help but we need to make sure improvements there are accessible as well (not just diagrams).

By "reasoning", if you mean why they are the way they are and require migrating a block to the latest version in a single step. It really is performance, I think, to avoid repeatedly parsing blocks, comparing, etc.

If you mean it how deprecations work still, I'll expand a little further below: (by the looks of your updated example, you're across most of this so please bare with me 🙏 )

Imagine we have two versions of a block:

Original
Updated #1

This is the simplest case for deprecations as there is only a single deprecation required.

Original --> Updated #1

Now the block is being updated again, to Updated #2, so we'd have three possible versions of the block;

Original
Updated #1
Updated #2

This means another deprecation is required. However, because deprecations aren't applied sequentially, the first deprecation also needs updating. So we'd have deprecations to perform the following:

Original --> Updated #2
Updated #1 --> Updated #2

With these deprecations, if the saved content for a block matched the Original version, the first deprecation is applied to migrate the latest version of the block. If the block's saved content didn't match the Original version, but did match the Updated #1 version, then the second deprecation would be applied.

Things get murkier if the saved content for a block could actually be the same for both Original and Updated #1. The first deprecation to match will be applied however the deprecations will be checked each time the block is processed in the editor, so it's possible that a migrated block might end up being migrated again, incorrectly.

This is where isEligible comes in. If a deprecation matches the saved content, it is only run if isEligible returns true. Sometimes this check might need updating on past deprecations when new versions of a block are added. Generally speaking though, it just needs to reliably determine if the current block is the version that deprecation is targeting.

In the current example, if both Original and Updated #1 versions of the block saved the same markup (or nothing), the deprecations need isEligible checks. The Original --> Updated #2 deprecation would need a check that would only return true for an Original version of the block. Likewise, the Updated #1 --> Updated #2 deprecation would need a check that only returned true for an Updated #1 version of the block.

On that note, it bring us to your updated example.

I have modified the example provided ... however, I'm not getting why the v1 migration is still run after v2, since the sequence should stop

There's a problem with the isEligible checks in the example. They are checking if an attribute has been migrated rather than whether the current version of the block is eligible to be migrated by the deprecation.

For v1, it should migrate when the block has a name and surname attribute. v2 should only migrate when there is a firstName and surname attribute combo.

With the current checks, v2 is determined to be eligible because lastName isn't set in the attributes but because the v2 attributes also don't define name that value isn't parsed into the block attributes and so isn't migrated by the v2 deprecation.

Then as the block is next validated, it doesn't match v2 anymore because it had the surname attribute migrated to lastName already. It then hits the isEligible for v1 and because that returns true if firstName wasn't set, which it couldn't be given v2's configuration, that deprecation is run.

This might make more sense if you added debug logs to the migrate and isEligible functions for each deprecation logging out the attributes object at each stage.

The example where the block has no content at all, and is relying more heavily on isEligible is a bit of an edge case.

So to get the example deprecation working, the isEligible functions need fixing. Something like:

// v2
    isEligible(attributes) {
        return (
            attributes.hasOwnProperty("firstName") &&
            attributes.hasOwnProperty("surname")
        );
    }

// v1
    isEligible(attributes) {
        return (
            attributes.hasOwnProperty("name") && attributes.hasOwnProperty("surname")
        );
    },

I appreciate your patience and time reading through all that. I'm hoping that it not only helps you but might help others that are wrestling with deprecations now or in the future 🤞

Hi, This issue has gone 30 days without any activity. This means it is time for a check-in to make sure it is still relevant. If you are still experiencing this issue with the latest versions, you can help the project by responding to confirm the problem and by providing any updated reproduction steps. Thanks for helping out.

@andreg I'm going to close this issue as the deprecations are working as intended. If you have any further questions, I'm happy to answer any I can.

WordPress / gutenberg