#56707 closed defect (bug) (fixed)
`register_block_type`'s `editor_script` handle fails if it is an array.
Reported by: | nendeb55 | Owned by: | Bernhard Reiter |
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | 6.1 |
Component: | Editor | Keywords: | has-testing-info has-patch has-unit-tests dev-reviewed |
Focuses: | Cc: |
Description
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.
Change History (32)
#2
@
2 years ago
- Component changed from General to Editor
- Milestone changed from Awaiting Review to 6.1
This ticket was mentioned in βSlack in #core by chaion07. βView the logs.
2 years ago
#4
@
2 years ago
- Keywords needs-testing added
@nendeb55 thank you for reporting this. We reviewed this ticket during a recent bug-scrub session and based on the feedback received we are updating it with the following changes:
- Adding keyword needs-testing
- @costdev has graciously volunteered to add testing instructions and test report later at his convenience. Thanks, Colin!
Cheers!
Props to @costdev
#5
@
2 years ago
- Keywords has-testing-info added
Testing Instructions
These steps define how to reproduce the issue, and indicate the expected behavior.
Steps to Reproduce
- Create a new file
wp-content/plugins/block-editor-script-test.php
with the following contents:<?php /** * Plugin Name: Block Editor Script Test * Author: WordPress Core Contributors * Author URI: https://make.wordpress.org/core * License: GPLv2 or later * Version: 1.0.0 */ add_action( 'init', 'wptest_register_hello_world_block' ); function wptest_register_hello_world_block() { $args = array( 'editor_script' => array( 'hello', 'world' ) ); register_block_type( 'wptest/hello-world', $args ); } add_action( 'admin_notices', 'wptest_show_editor_script' ); function wptest_show_editor_script() { $registry = WP_Block_Type_Registry::get_instance(); $registered = $registry->get_all_registered(); $editor_script = json_encode( $registered['wptest/hello-world']->editor_script ); echo '<div class="notice notice-info"><p>' . $editor_script . '</p></div>'; }
- Navigate to
Plugins > Installed plugins
. - π Activate the Block Editor Script Test plugin.
Expected Results
When reproducing a bug:
- β The admin notice should show:
null
[]
When testing a patch to validate it works as expected:
- β The admin notice should show:
null
["hello","world"]
Test Report Icons:
π <= Indicates where issue ("bug") occurs.
β <= Behavior is expected.
β <= Behavior is NOT expected.
#6
@
2 years ago
- Keywords dev-feedback added
The expected results for a possible patch needs discussion.
As Core did not previously protect against a non-string value, it was possible to register a block with editor_script
set to an array rather than the documented string|null
types.
Pinging @gziolo as the committer of [54155]. Greg, βthis condition results in an array editor_script
being rejected outright.
Instead, if editor_script
is an array, should we set editor_script_handles
to the same array? Should this happen for all the deprecated properties, or should these just return void
?
#7
@
2 years ago
Instead, if editor_script is an array, should we set editor_script_handles to the same array? Should this happen for all the deprecated properties, or should these just return void?
I'm curious how it worked correctly in the first place, but let's address it gracefully.
@costdev, I believe what you suggested makes perfect sense, and we can apply it to all script types to make the patch simple. I guess all we need to change is this lines:
When an array is passed, it would be as simple as:
$this->{$new_name} = $value;
This ticket was mentioned in βSlack in #core by audrasjb. βView the logs.
2 years ago
#9
@
2 years ago
@gziolo Thanks for the tip.
How about sending them only when the array comes in?
βhttps://github.com/WordPress/wordpress-develop/blob/70dedb086602287f4e0ee97fbf477adda8310963/src/wp-includes/class-wp-block-type.php#L340-L352
<?php public function __set( $name, $value ) { if ( ! in_array( $name, $this->deprecated_properties ) ) { $this->{$name} = $value; return; } $new_name = $name . '_handles'; if ( ! is_string( $value ) ) { $this->{$new_name} = $value; }else{ $this->{$new_name}[0] = $value; } }
#10
@
2 years ago
Maybe is_array is better than is_string for judging.
<?php public function __set( $name, $value ) { if ( ! in_array( $name, $this->deprecated_properties ) ) { $this->{$name} = $value; return; } $new_name = $name . '_handles'; if ( is_array( $value ) ) { $this->{$new_name} = $value; }else{ if ( ! is_string( $value ) ) { return; } $this->{$new_name}[0] = $value; } }
#11
@
2 years ago
@nendeb55 Slight modification to minimize nesting:
<?php public function __set( $name, $value ) { if ( ! in_array( $name, $this->deprecated_properties ) ) { $this->{$name} = $value; return; } $new_name = $name . '_handles'; if ( is_array( $value ) ) { $this->{$new_name} = $value; return; } if ( ! is_string( $value ) ) { return; } $this->{$new_name}[0] = $value; }
@zieladam, as Greg is on leave, you're up! π (If someone else should be pinged, please do so!)
Is there any chance that $this->{$new_name}
will already have some entries that must stay there? In other words, is $this->{$new_name} = $value
safe to use, or should we be prepending $value
to $this->{$new_name}
instead?
#12
@
2 years ago
@costdev Thanks for the improvement.
I have tested it with NULL or empty value inside $value and it seems to work fine.
If you are concerned I can add this code.
<?php if( in_array( NULL, $value, TRUE ) ){ return; }
#13
@
2 years ago
@nendeb55 I may be mistaken on this, so I defer to others more experienced with this feature, but I would assume that $value
should either be a string, or a string array.
The documentation says $value
is mixed
, but for deprecated properties, only strings are currently supported. Therefore, if we are going to add back support for arrays, I assume that these should only be string arrays.
If that's the case, and we want to remove any incorrect data, then we could do this:
<?php if ( is_array( $value ) ) { $this->{$new_name} = array_filter( $value, 'is_string' ); }
or:
<?php if ( is_array( $value ) ) { $filtered = array_filter( $value, 'is_string' ); if ( count( $filtered ) !== count( $value ) ) { _doing_it_wrong( __METHOD__, sprintf( /* translators: %s: The '$value' argument. */ __( 'The %s argument must be a string or a string array.' ), '<code>$value</code>' ), 'x.x.x', ); } $this->{$new_name} = $filtered; }
#14
@
2 years ago
@costdev Thank you for the improvement.
I hope this is the kind of code you're looking for to organize it.
<?php public function __set( $name, $value ) { if ( ! in_array( $name, $this->deprecated_properties ) ) { $this->{$name} = $value; return; } $new_name = $name . '_handles'; if ( is_array( $value ) ) { $filtered = array_filter( $value, 'is_string' ); if ( count( $filtered ) !== count( $value ) ) { _doing_it_wrong( __METHOD__, sprintf( /* translators: %s: The '$value' argument. */ __( 'The %s argument must be a string or a string array.' ), '<code>$value</code>' ), '6.1.0', ); } $this->{$new_name} = $filtered; return; } if ( ! is_string( $value ) ) { return; } $this->{$new_name}[0] = $value; }
Tested. It worked correctly.
Will this be reflected in WordPress 6.1?
#15
@
2 years ago
Thanks @nendeb55!
Will this be reflected in WordPress 6.1?
As this is a regression introduced in 6.1, I hope so. We'll need some feedback from others who worked on this feature and I believe it will need to be patched in the Gutenberg GitHub repository. I'll highlight this ticket with the 6.1 release squad to help move this forward.
This ticket was mentioned in βSlack in #core by chaion07. βView the logs.
2 years ago
This ticket was mentioned in βPR #3487 on βWordPress/wordpress-develop by β@Bernhard Reiter.
2 years ago
#17
- Keywords has-patch has-unit-tests added
Based on prior work by @nendeb and @costdev in 56707.
Props @nendeb @costdev @gziolo
### TODO
- https://core.trac.wordpress.org/ticket/56707#comment:7 Fix getter (see currently failing unit test and @gziolo's [comment])
- [ ] Ideally test setter separately from getter
- [ ] Make sure REST API endpoint works -- add tests?
- [ ] Update PHPDoc
Trac ticket: https://core.trac.wordpress.org/ticket/56707
β@Bernhard Reiter commented on βPR #3487:
2 years ago
#18
I've added unit test coverage for the issue discussed in the conversation starting at βhttps://github.com/WordPress/wordpress-develop/pull/3487#discussion_r999249106. They're expected to fail until we settle on a resolution (see βhttps://github.com/WordPress/wordpress-develop/pull/3487#discussion_r999569253).
This ticket was mentioned in βSlack in #core by bernhardreiter. βView the logs.
2 years ago
β@Bernhard Reiter commented on βPR #3487:
2 years ago
#20
Trying to draft a commit message:
Blocks: Allow arrays for deprecated asset types in block registration. Follow-up [54155]. 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`. Props nendeb55, costdev, gziolo. Fixes #56707.
This ticket was mentioned in βSlack in #core by bernhardreiter. βView the logs.
2 years ago
β@Bernhard Reiter commented on βPR #3487:
2 years ago
#22
Looks good to me π
Thank you very much, @SergeyBiryukov! π
(I think your approval just coincided with my 9bc56a53bd3b4cbc3bfc4e78bf5637eec4b4ed14 which I pushed to address βhttps://github.com/WordPress/wordpress-develop/pull/3487#discussion_r1000460007 -- not sure you saw that commit π
)
β@SergeyBiryukov commented on βPR #3487:
2 years ago
#23
(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 π
This ticket was mentioned in βSlack in #core by audrasjb. βView the logs.
2 years ago
β@audrasjb commented on βPR #3487:
2 years ago
#25
There's some coding standards issues to fix though :)
#26
@
2 years ago
As per today's bug scrub:
There's some coding standards issues to fix though. Other wise, it looks good to go.
#28
@
2 years ago
- Owner set to Bernhard Reiter
- Resolution set to fixed
- Status changed from new to closed
In 54670:
β@Bernhard Reiter commented on βPR #3487:
2 years ago
#30
#31
@
2 years ago
- Keywords dev-reviewed added; needs-testing dev-feedback removed
Noting that this had approval on the PR by multiple Core committers (βSergeyBiryukov βaudrasjb βspacedmonkey) and thus qualified for merging to the 6.1 release branch.
Thanks all!
Moving to 6.1 milestone to investigate this possible regression.