Skip to content

Conversation

@talldan
Copy link
Contributor

@talldan talldan commented Jun 28, 2019

Description

Alternative attempt to fix #16328. Unfinished and unlikely to be the right fix, but this PR should hopefully generate some discussion about the right way to fix the issue.

Background
After some digging into the above issue, I discovered that the supports settings for blocks relies on the registerBlockType filter. This filter adds an extra anchor attribute to blocks that support anchors.

Unfortunately the registerBlockType filter only runs on the main block settings, not deprecated block settings. When falling back to an older deprecation the attribute is not present and is never sourced from the HTML. In the case reported in the related issue, this caused a validation issue.

Other supports settings (e.g. align) also don't work with deprecations, but don't cause validation issues since data- attributes are ignored by the validator.

The fix here is to also run the registerBlockType filter over each object in the deprecation array.

How has this been tested?

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@talldan talldan added [Type] Bug An existing feature does not function as intended [Feature] Block API API that allows to express the block paradigm. labels Jun 28, 2019
@talldan talldan self-assigned this Jun 28, 2019
@talldan
Copy link
Contributor Author

talldan commented Jun 28, 2019

Not sure this is the right thing to do as it might have undesired consequences for other implementors of this filter (e.g. for plugins).

@talldan talldan mentioned this pull request Jun 28, 2019
5 tasks
@talldan talldan requested review from ajitbohra, nerrad and ntwb as code owners July 2, 2019 11:12
],
};

addFilter( 'blocks.registerBlockType', 'core/blocks/without-title', ( settings ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the filter after the test to ensure it doesn't mess with other tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already handled in an afterEach on line 317.

@aduth
Copy link
Member

aduth commented Jul 2, 2019

Not sure this is the right thing to do as it might have undesired consequences for other implementors of this filter (e.g. for plugins).

Might be worth doing a look over keyword matches from the plugin repository using wpdirectory.net.

Example: https://wpdirectory.net/search/01DESKYX62AMYKSZE8KJ5FBQKF

@talldan
Copy link
Contributor Author

talldan commented Jul 2, 2019

Might be worth doing a look over keyword matches from the plugin repository using wpdirectory.net.

Example: https://wpdirectory.net/search/01DESKYX62AMYKSZE8KJ5FBQKF

Thanks for the link @aduth. It mostly looks ok, there's quite a bit of minified code, though.

There were a couple of plugins that use the filter to completely replace/enhance a block. For example, Jetpack:
https://github.com/Automattic/jetpack/blob/4c63a49fc0139c3296248e09ee69dedb537e024f/extensions/blocks/videopress/editor.js

This change would result in a bunch of properties being added to the deprecated settings that would go unused (deprecated, edit, transform). Probably not harmful, but perhaps it'd be better for the code in this PR to pick just supports, save and attributes from the filter return value?

Will also add an issue for Jetpack on how they're setting that deprecated value. If the video block is deprecated in core, that could cause some issues!

@youknowriad
Copy link
Contributor

Let's ship this. The plugin releases will allow us to detect any potential issue with it we might have missed.

@talldan
Copy link
Contributor Author

talldan commented Jul 4, 2019

👍

@talldan talldan merged commit 72697ce into master Jul 4, 2019
@talldan talldan deleted the fix/hooks-not-run-on-deprecations branch July 4, 2019 10:30
@youknowriad youknowriad added this to the Gutenberg 6.1 milestone Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Block API API that allows to express the block paradigm. [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Group blocks won't validate after update to 6.0

4 participants