Fix block validation errors caused by amp-fit-text deprecation

[westonruter]

Summary

I noticed that the deprecation of amp-fit-text (#5729) started causing block validation errors to show up when the Twenty Seventeen theme is active as of WordPress 5.8.3 and 5.9. In 5.9 when I open the block inserter and select the Patterns tab and select Twenty Seventeen from the dropdown, I get these errors in the console:

And the Services pattern shows those errors in the preview:

In WordPress 5.8.3 I see these errors and even more just when I open the editor without even opening the block inserter, with many mentioning <amp-fit-text> explicitly:

I have no idea why this would be happening since the isEligible function for our amp-fit-text deprecation definition is returning false. Nevertheless, I did notice that we were adding up six identical copies of our deprecation definition as the filter is being applied multiple times it seems. I found that ensuring the definition is only added once will prevent the errors from happening. I also found that using push instead of unshift also fixes the issue. Since our amp-fit-text deprecation should be relatively rare, I decided to use push as well.

Any insights into why this is happening and why the fix works would be much appreciated!

For reference, here is a paragraph block using amp-fit-text:

<!-- wp:paragraph {"ampFitText":true} -->
<amp-fit-text layout="fixed-height" min-font-size="5" max-font-size="100" height="100"><p>This is fit-text!!</p></amp-fit-text>
<!-- /wp:paragraph -->

This should still be migrated successfully as follows:

<!-- wp:paragraph -->
<p>This is fit-text!!</p>
<!-- /wp:paragraph -->

Checklist

• My code is tested and passes existing tests.
• My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

[github-actions]

Plugin builds for d6f2460 are ready 🛎️!

• Download development build
• Download production build

[westonruter]

Here's the definition of the problematic block pattern in Twenty Seventeen: https://github.com/WordPress/wordpress-develop/blob/2bbe1b7f40bda13fc6625aad6e3f1c0913566b39/src/wp-content/themes/twentyseventeen/inc/block-patterns.php#L145-L179

I think it may be causing problems because "lineHeight":"2.5" is deprecated (I think?) and so it causes the deprecation logic to run.

[delawski]

Nevertheless, I did notice that we were adding up six identical copies of our deprecation definition as the filter is being applied multiple times it seems.

This, I think, is caused by the fact that the blocks.registerBlockType filter is run multiple times (once per each deprecation plus one time initially). I was having trouble fully understanding why is it done this way but I'll circle back with a fresh look tomorrow (see: WordPress/gutenberg#34299).

I also found that using push instead of unshift also fixes the issue. Since our amp-fit-text deprecation should be relatively rare, I decided to use push as well.

Here's a docs fragment that explains how deprecations are handled (the emphasis at the end is mine):

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.

For blocks with multiple deprecations, it may be easier to save each deprecation to a constant with the version of the block it applies to, and then add each of these to the block’s deprecated array. The deprecations in the array should be in reverse chronological order. This allows the block editor to attempt to apply the most recent and likely deprecations first, avoiding unnecessary and expensive processing.

It seems that adding our deprecation to the end of the list is reasonable since it will run after all core deprecations are executed (and thus hopefully cleaning up the mess that's not really AMP-related).

Then I did a quick test to see if the console error you noticed has anything to do with the plugin. I've deactivated the AMP plugin, and then pasted an offending block markup from Twenty Seventeen pattern into a post content in the code editor mode:

<!-- wp:paragraph {"style":{"typography":{"fontSize":21,"lineHeight":"2.5"}}} -->
<p style="font-size:21px;line-height:2.5"><p style="font-size:21px"><a href="#">' . __( 'Branding', 'twentyseventeen' ) . ' →</a><br><a href="#">' . __( 'Web Design', 'twentyseventeen' ) . ' →</a><br><a href="#">' . __( 'Web Development', 'twentyseventeen' ) . ' →</a></p></p>
<!-- /wp:paragraph -->

After saving the post and refreshing, I received the same console error as reported above:

And this has made me wonder even more as it appears that the amp-fit-text deprecations fixes (?) the non-AMP-related Twenty Seventeen issue. And also it seems that the core deprecations don't really clean up the mess they are supposed to clean (contrary to what I thought earlier).

It looks like the core/paragraph deprecations in conjunction with the recent changes in a way the block.registerBlockType hook is handled (WordPress/gutenberg#34299) generate this error.

What I don't know is if this PR actually fixes the issue or just somehow masks it.

[dhaval-parekh]

✅ QA Passed.

After changes, No longer able to reproduce block validation errors for paragraph block in "Twenty Seventeen" theme is active in WordPress 5.9

Also, It is migrating paragraph block using amp-fit-text to normal paragraph block without any issue.