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

register_block_type: Accept editor_script array for back-compat #3487

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Oct 18, 2022

Based on prior work by @nendeb and @costdev in 56707. Quoting that ticket:

I had multiple handles for editor_script and passed the handles to editor_script as an array.
It works fine in WP6.0 but fails in WP61 and the block becomes unusable.
I know that editor_script was deprecated in WP6.1 and changed to editor_script_handles, but please fix it because it is not backward compatible that editor_script cannot be passed as an array.

Props @nendeb @costdev @gziolo

TODO

  • Fix getter (see currently failing unit test and @gziolo's comment)
  • Ideally test setter separately from getter Probably okay as-is; they're two faces of the same medal.
  • Make sure REST API endpoint works -- add tests?
  • Update PHPDoc

Trac ticket: https://core.trac.wordpress.org/ticket/56707


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@ockham ockham force-pushed the fix/register-block-type-accept-editor-script-array branch from 44c2547 to 15dbfb7 Compare October 19, 2022 09:17
@ockham ockham self-assigned this Oct 19, 2022
@ockham ockham marked this pull request as ready for review October 19, 2022 09:58
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

There is one edge case to address that I explained in https://github.com/WordPress/wordpress-develop/pull/3487/files#r999249106. Otherwise, it's looking good in terms of covering the legacy undocumented behavior. Nice team work on the patch 💯

@ockham
Copy link
Contributor Author

ockham commented Oct 19, 2022

I've added unit test coverage for the issue discussed in the conversation starting at #3487 (comment). They're expected to fail until we settle on a resolution (see #3487 (comment)).

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Excellent work. Thank you addressing all my feedback. It wasn’t simple to tackle with different constraints in the REST API and in how the block type was handled in the past. In my eyes, it’s ready to land.

tests/phpunit/tests/blocks/register.php Show resolved Hide resolved
@ockham
Copy link
Contributor Author

ockham commented Oct 20, 2022

Trying to draft a commit message:

Blocks: Allow arrays for deprecated asset types in block registration.

In `register_block_type`, continue to allow passing arrays as the `editor_script`, `script`, `view_script`, `editor_style`, and `style` arguments. Note that those fields were soft-deprecated in favor of their `_handles` counterparts in [54155], which would allow specifying multiple items. At the same time, the deprecated fields were limited to `string` or `null`.

However, this broke existing code that passed an array as one of those arguments. For backwards compatibility, this change thus restores the previous behavior. It is implemented in `WP_Block_Type` as a pair of `__get()` and `__set()` methods that wrap around the corresponding `_handles` members, which are arrays of strings.

It also affects the REST API endpoint for block types. The latter’s schema has never allowed for anything other than `string` or `null` for any of those fields. For this reason, it now returns the first element of the array stored in the corresponding `_handles` member in `WP_Block_Type`.

Follow-up [54155].
Props nendeb55, costdev, gziolo, spacedmonkey, mukesh27, sergeybiryukov, audrasjb.
Fixes #56707.

@ockham ockham force-pushed the fix/register-block-type-accept-editor-script-array branch from f53fc2f to 7c1c7f7 Compare October 20, 2022 09:45
Comment on lines 395 to 399
$this->assertSame( 'hello', $data['editor_script'] );
$this->assertSame( 'gutenberg', $data['script'] );
$this->assertSame( 'foo', $data['view_script'] );
$this->assertSame( 'guten', $data['editor_style'] );
$this->assertSame( 'out', $data['style'] );
Copy link
Member

Choose a reason for hiding this comment

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

Test methods with multiple assertions need a message parameter for each assertion, per Core Handbook - Writing PHPUnit Tests - Using Assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 9bc56a5 😊

Copy link
Member

@SergeyBiryukov SergeyBiryukov left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@ockham
Copy link
Contributor Author

ockham commented Oct 20, 2022

Looks good to me 👍

Thank you very much, @SergeyBiryukov! 🎉

(I think your approval just coincided with my 9bc56a5 which I pushed to address #3487 (comment) -- not sure you saw that commit 😅 )

@SergeyBiryukov
Copy link
Member

(I think your approval just coincided with my 9bc56a5 which I pushed to address #3487 (comment) -- not sure you saw that commit 😅 )

Looks even better now 😄

@audrasjb
Copy link
Contributor

There's some coding standards issues to fix though :)

@ockham
Copy link
Contributor Author

ockham commented Oct 24, 2022

Merged to Core in r54670, backported to the 6.1 branch in r54671.

@ockham ockham closed this Oct 24, 2022
@ockham ockham deleted the fix/register-block-type-accept-editor-script-array branch October 24, 2022 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

6 participants