Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#56707 closed defect (bug) (fixed)

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

Reported by: nendeb55's profile nendeb55 Owned by: bernhard-reiter's profile 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)

#1 @nendeb55
2 years ago

  • Version set to trunk

#2 @desrosj
2 years ago

  • Component changed from General to Editor
  • Milestone changed from Awaiting Review to 6.1

Moving to 6.1 milestone to investigate this possible regression.

This ticket was mentioned in ​Slack in #core by chaion07. ​View the logs.


2 years ago

#4 @chaion07
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:

  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

#5 @costdev
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

  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.

#6 @costdev
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?

Last edited 2 years ago by costdev (previous) (diff)

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

​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;
Version 0, edited 2 years ago by gziolo (next)

This ticket was mentioned in ​Slack in #core by audrasjb. ​View the logs.


2 years ago

#9 @nendeb55
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 @nendeb55
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 @costdev
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 @nendeb55
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 @costdev
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 @nendeb55
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 @costdev
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

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 @audrasjb
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.

#27 @audrasjb
2 years ago

Coding standards issues were fixed in the PR πŸ‘

#28 @Bernhard Reiter
2 years ago

  • Owner set to Bernhard Reiter
  • Resolution set to fixed
  • Status changed from new to closed

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.

#29 @Bernhard Reiter
2 years ago

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.

​@Bernhard Reiter commented on ​PR #3487:


2 years ago
#30

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

#31 @Bernhard Reiter
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!

This ticket was mentioned in ​Slack in #core by bernhardreiter. ​View the logs.


2 years ago

Note: See TracTickets for help on using tickets.