`register_block_type`'s `editor_script` handle fails if it is an array.

[nendeb55]

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.

[desrosj]

Moving to 6.1 milestone to investigate this possible regression.

[chaion07]

@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:

1. Adding keyword needs-testing
2. @costdev has graciously volunteered to add testing instructions and test report later at his convenience. Thanks, Colin!

Cheers!

Props to @costdev

[costdev]

Testing Instructions

These steps define how to reproduce the issue, and indicate the expected behavior.

Steps to Reproduce

1. 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>';
   }
   

2. Navigate to Plugins > Installed plugins.
3. 🐞 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.

[costdev]

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?

[gziolo]

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:

​https://github.com/WordPress/wordpress-develop/blob/70dedb086602287f4e0ee97fbf477adda8310963/src/wp-includes/class-wp-block-type.php#L346-L351

When an array is passed, it would be as simple as:

$this->{$new_name} = $value;

I'm not sure how we can support the getter, though. It needs to work with the REST API endpoint, where it has to return a string or null.

[nendeb55]

@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;
                }
        }

[nendeb55]

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;
                 }
         }

[costdev]

@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?

[nendeb55]

@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;
}

[costdev]

@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;
}

[nendeb55]

@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?

[costdev]

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.

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

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).

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.

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 😅 )

(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 😄

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

[audrasjb]

As per today's bug scrub:
There's some coding standards issues to fix though. Other wise, it looks good to go.

[audrasjb]

Coding standards issues were fixed in the PR 👍

[Bernhard Reiter]

In 54670:

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.

[Bernhard Reiter]

In 54671:

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.
Merges [54670] to the 6.1 branch.
Fixes #56707.

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

[Bernhard Reiter]

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!