Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated behaviors syntax #57165

Merged
merged 5 commits into from
Jan 2, 2024
Merged

Conversation

michalczaplinski
Copy link
Contributor

@michalczaplinski michalczaplinski commented Dec 18, 2023

What?

Removes the deprecated behaviors syntax.

Behaviors UI was originally introduced in #49972. It was meant to be a way to add reusable pieces of interactivity to the frontend of sites.

That implementation was removed in #53851 and #54509 and never merged into Core. Backwards-compatibility layer was added for users of the GB plugin.

Why?

It should have already been deprecated in 17.0 because we only offer backwards-compatibility for 2 releases 🙂.

How?

Removing 2 files and references to behaviors and legacyLightboxSettings.

Testing Instructions

  1. Open a post or page.
  2. Insert an image block.
  3. The "Expand on click" functionality should continue to work fine.
  4. Try to insert the block that contains the removed syntax and it should NOT work anymore:
    <!-- wp:image {"behaviors":{"lightbox":{"enabled":true}},"id":157,"sizeSlug":"large","linkDestination":"none"} -->
    <figure class="wp-block-image size-large"><img src="http://gutenberg.local/wp-content/uploads/2023/11/alan-hardman-SU1LFoeEUkk-unsplash-1024x678.jpg" alt="" class="wp-image-157"/></figure>
    <!-- /wp:image -->
    

Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/blocks.php
❔ lib/class-wp-theme-json-gutenberg.php
❔ lib/load.php

@michalczaplinski michalczaplinski added [Type] Breaking Change For PRs that introduce a change that will break existing functionality [Block] Image Affects the Image Block Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Block API API that allows to express the block paradigm. labels Dec 18, 2023
@michalczaplinski
Copy link
Contributor Author

Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

There is no need to sync anything to WordPress Core. The removed/ updated files were never part of Core in the first place because Behaviors didn't ship in 6.4.

@artemiomorales
Copy link
Contributor

artemiomorales commented Dec 19, 2023

For reference, the backwards compatibility was added in #54071

Copy link
Contributor

@artemiomorales artemiomorales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me and appears to get rid of the backwards compatibility that should be removed.

However, I'll note that the following testing instructions are incorrect:

  1. Try to insert the block that contains the removed syntax and it should NOT work anymore:
<!-- wp:image {"behaviors":{"lightbox":{"enabled":true}},"id":157,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://gutenberg.local/wp-content/uploads/2023/11/alan-hardman-SU1LFoeEUkk-unsplash-1024x678.jpg" alt="" class="wp-image-157"/></figure>
<!-- /wp:image -->

At the block level, the behaviors syntax is still supported and migrated via the image's deprecated.js, logic which I believe should still remain intact.

Rather than handling that scenario, this PR removes the support for any custom block that had added the behaviors block support. It also removes the backwards compatibility for anyone who had enabled the lightbox using the prior version of the UI in the Site Editor.

With that caveat in mind, I think this PR is good to go @michalczaplinski 👍

@michalczaplinski
Copy link
Contributor Author

Thanks for the review, @artemiomorales

As far as I can tell, the behaviors syntax will not be supported for the Image block when we remove the compat layer here and here.

output_904f74.mp4

So, the block migration does not really do anything useful at the moment. It probably could just be removed as well. What do you think?

@artemiomorales
Copy link
Contributor

So, the block migration does not really do anything useful at the moment. It probably could just be removed as well. What do you think?

@michalczaplinski Hmm, so I believe the migration code doesn't actually run until you make a modification to the post before pressing Update. If you do that, you'll see that the behavior attribute get migrated to the new syntax:

behaviors-deprecation.mp4

With that in mind, the code in deprecated.js serves to move users to v8 of the of the block attributes.

That being said, I initially thought we would need to keep this to prevent breakage and inconsistencies in folks' block versions; as I'm looking at the deprecation section of the handbook though, I think you're right and we may be able to remove the v8 code from deprecated.js since it appears the block versions are just migrated and determined at runtime.

What do you think? I could look at this more closely tomorrow.

@michalczaplinski
Copy link
Contributor Author

michalczaplinski commented Dec 20, 2023

the migration code doesn't actually run until you make a modification to the post

That's weird! I didn't realize that the migration does not run until the post Update. I thought it should have been running on the initial publish as well.

I think that If we're removing the compat layer, then we can also remove the deprecations but I don't know if there's a precedent for that.

@ajlende Does that sound fine for you?

@artemiomorales
Copy link
Contributor

artemiomorales commented Dec 20, 2023

I think that If we're removing the compat layer, then we can also remove the deprecations but I don't know if there's a precedent for that.

Ok pinging @youknowriad @oandregal for feedback. Should we remove the v8 version in the image's deprecated.js if we're no longer supporting behaviors and no longer have a need for the deprecation?

@ajlende
Copy link
Contributor

ajlende commented Dec 21, 2023

Hmm, so I believe the migration code doesn't actually run until you make a modification to the post before pressing Update.

It runs on parse, but I'm a bit surprised that parse never gets called on the initial save. I thought it used to be called even when you switched out of code mode, but I could also be misremembering.

I think that If we're removing the compat layer, then we can also remove the deprecations but I don't know if there's a precedent for that.

@ajlende Does that sound fine for you?

If there's a scenario where old attributes/markup could have been saved and those old attributes/markup are no longer valid, then you need a deprecation.

@oandregal
Copy link
Member

Hmm, so I believe the migration code doesn't actually run until you make a modification to the post before pressing Update.

Hi, I haven't worked much with deprecations, but this is my understanding:

  • Deprecations run when the block is invalid.
  • An existing block having an unknown attribute does not make a block invalid. You can try by adding one attribute to the block using the code editor and switch to the visual editor (this also triggers the parse).

If we remove the deprecation, what would happen is that old blocks that used the behaviors attribute would maintain the attribute. It's block comment's pollution – no big deal, also not ideal.

What's important is that changing implementations should keep things working for the user. Ideally, we should maintain the deprecation and use it to migrate the block attributes to the new implementation.

An example of the ideal scenario:

  • I set the lightbox for an image using the first behaviors' implementation. And I haven't touched the post since.
  • After this PR (and all before), the image still has the lightbox enabled – using the new implementation.

Sorry I cannot provide more a more concrete answer. Hope this helps!

@artemiomorales
Copy link
Contributor

Ok got it! Looks like the deprecation stays @michalczaplinski 👍

@michalczaplinski
Copy link
Contributor Author

Thanks for your input folks!

I'm merging it as is in that case (keeping the deprecation).

@michalczaplinski michalczaplinski merged commit 50a8314 into trunk Jan 2, 2024
51 checks passed
@michalczaplinski michalczaplinski deleted the remove-deprecated-behaviors branch January 2, 2024 17:26
@github-actions github-actions bot added this to the Gutenberg 17.5 milestone Jan 2, 2024
@getdave getdave added the Needs PHP backport Needs PHP backport to Core label Jan 15, 2024
@getdave getdave removed the Needs PHP backport Needs PHP backport to Core label Jan 22, 2024
@getdave
Copy link
Contributor

getdave commented Jan 22, 2024

✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR does not require a backport for WP 6.5.

This is due to this comment (in the PR description):

That implementation was removed in #53851 and #54509 and never merged into Core

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Block] Image Affects the Image Block [Feature] Block API API that allows to express the block paradigm. [Type] Breaking Change For PRs that introduce a change that will break existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants