Make WordPress Core

Opened 4 years ago

Closed 7 weeks ago

#52592 closed defect (bug) (fixed)

PHP warning when the label property is missing from register_block_style

Reported by: poena's profile poena Owned by: audrasjb's profile audrasjb
Milestone: 6.8 Priority: normal
Severity: normal Version: 5.3
Component: Editor Keywords: has-patch needs-testing has-unit-tests
Focuses: Cc:

Description

If I register a block style using the PHP function
register_block_style, and I leave out the label property, there is a PHP notice:

Notice:  Undefined index: label in /var/www/src/wp-includes/script-loader.php on line 2312

The label is required, but the class does not check if a label exists or not, see:
https://core.trac.wordpress.org/browser/tags/5.6.1/src/wp-includes/class-wp-block-styles-registry.php#L43

In comparison, if I leave out the name property, a _doing_it_wrong message is shown.
I think a _doing_it_wrong should be shown for the label too.

Steps to reproduce the issue:

Register an incomplete block style in a theme or plugin file using register_block_style.

register_block_style(
    'core/quote',
    array(
        'inline_style' => '.wp-block-quote.is-style-blue-quote { color: blue; }',
    )
);

Confirm that the doing it wrong message for the name is showing.

Add the name property:

register_block_style(
    'core/quote',
    array(
	'name'  => 'test',
        'inline_style' => '.wp-block-quote.is-style-blue-quote { color: blue; }',
    )
);

Open the block editor. Confirm that a PHP notice for the label is showing.

Attachments (1)

52592-tests.diff (3.7 KB) - added by Rahmohn 7 months ago.
Refactoring and adding tests

Download all attachments as: .zip

Change History (16)

#1 @poena
9 months ago

I tested again today on 6.6 RC3 using PHP 8.1.23, and what was a notice is now a warning:

Warning:  Undefined array key "label" in wp-includes/script-loader.php on line 2716


#2 @poena
9 months ago

  • Summary changed from PHP notice when the label property is missing from register_block_style to PHP warning when the label property is missing from register_block_style

This ticket was mentioned in PR #7008 on WordPress/wordpress-develop by @poena.


9 months ago
#3

  • Keywords has-patch added

This PR updates the register method inside WP_Block_Styles_Registry:
Adds a check for the required label style property.
If the label is not a string or is not set, display a doing_it_wrong notice, and return early.

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

#4 @poena
9 months ago

  • Keywords needs-testing added

Testing instructions for PR7008:

Enabling displaying or logging of PHP errors/warnings/notices.
Add the following code inside twentytwentyfour_block_styles() in functions.php of the theme Twenty Twenty-Four:

register_block_style(
    'core/quote',
    array(
	'name'  => 'test',
        'inline_style' => '.wp-block-quote.is-style-blue-quote { color: blue; }',
    )
);

The notice added in the PR should be present on the front and the admin area, but you may need to refresh the page first.

Notice: Function WP_Block_Styles_Registry::register was called incorrectly. Block style label must be a string. Please see Debugging in WordPress for more information. (This message was added in version 6.7.0.) in /var/www/src/wp-includes/functions.php on line 6085
Last edited 9 months ago by poena (previous) (diff)

#5 @poena
9 months ago

  • Milestone changed from Awaiting Review to 6.7

#6 @poena
9 months ago

  • Keywords needs-unit-tests added

#7 @poena
9 months ago

I tried but was not able to figure out how to create a new test to check that if the label is missing, the style is not registered and the doing_it_wrong message is triggered.

#8 @Rahmohn
7 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Hi @poena

I've added a test covering the missing/invalid label and the _doing_it_wrong trigger. Also, I've refactored a bit the test you added:

<?php
/**
 * Should accept valid string style label.
 * The registered style should have the same label.
 *
 * @ticket 52592
 *
 * @covers WP_Block_Styles_Registry::register
 * @covers WP_Block_Styles_Registry::is_registered
 * @covers WP_Block_Styles_Registry::get_registered_styles_for_block
 */
public function test_register_block_style_with_string_style_label() {
        $name             = 'core/paragraph';
        $style_properties = array(
                'name'  => 'fancy',
                'label' => 'Fancy',
        );
        $result           = $this->registry->register( $name, $style_properties );

        $this->assertTrue( $result, 'The block style should be registered when the label is a valid string.' );
        $this->assertTrue(
                $this->registry->is_registered( $name, 'fancy' ),
                'The block type should have the block style registered when the label is valid.'
        );
        $this->assertEquals(
                $style_properties['label'],
                $this->registry->get_registered_styles_for_block( $name )['fancy']['label'],
                'The registered block style should have the same label.'
        );
}

/**
 * Should not accept invalid style label.
 *
 * @ticket 52592
 *
 * @covers WP_Block_Styles_Registry::register
 * @covers WP_Block_Styles_Registry::is_registered
 */
public function test_register_block_style_with_invalid_style_label() {
        $name             = 'core/paragraph';
        $style_properties = array(
                'name' => 'fancy',
        );

        $this->setExpectedIncorrectUsage( 'WP_Block_Styles_Registry::register' );

        $result           = $this->registry->register( $name, $style_properties );
        $this->assertFalse( $result, 'The block style should not be registered when the label property is missing.' );
        $this->assertFalse(
                $this->registry->is_registered( $name, 'fancy' ),
                'The block type should not have the block style registered when the label property is missing.'
        );

        $style_properties['label'] = ['Fancy'];

        $result           = $this->registry->register( $name, $style_properties );
        $this->assertFalse( $result, 'The block style should not be registered when the label property is not a string.' );
        $this->assertFalse(
                $this->registry->is_registered( $name, 'fancy' ),
                'The block type should not have the block style registered when the label property is not a string.'
        );
}

I am attaching the diff file but I can create a new PR with your and my changes. Just let me know.

@Rahmohn
7 months ago

Refactoring and adding tests

@aaronrobertshaw commented on PR #7008:


6 months ago
#9

Thanks for putting this together @carolinan 👍

I'm not sure about the current approach. It could be a breaking change for sites that have a block style registered without a label in the style properties and enqueue the block style's CSS via a separate stylesheet.

The docs do note that the label property is required but this doesn't appear to have been enforced at all.

The properties of the style array must include name and label:

  • name: The identifier of the style used to compute a CSS class.
  • label: A human-readable label for the style.

Rather than replace one notice for another using doing_it_wrong, I think the expected behaviour here would be for the label to fallback to the name property if not supplied.

<details>
<summary>Maybe something as simple as this?</summary>

  • src/wp-includes/class-wp-block-styles-registry.php

    diff --git a/src/wp-includes/class-wp-block-styles-registry.php b/src/wp-includes/class-wp-block-styles-registry.php
    index 2d16e0e034..9a990173b4 100644
    a b final class WP_Block_Styles_Registry { 
    9393                $block_style_name = $style_properties['name'];
    9494                $block_names      = is_string( $block_name ) ? array( $block_name ) : $block_name;
    9595
     96                // Ensure there is a label defined.
     97                if ( empty( $style_properties['label'] ) ) {
     98                        $style_properties['label'] = $block_style_name;
     99                }
     100
    96101                foreach ( $block_names as $name ) {
    97102                        if ( ! isset( $this->registered_block_styles[ $name ] ) ) {
    98103                                $this->registered_block_styles[ $name ] = array();

</details>

The docs could probably remain as they are to encourage "proper use" of register_block_style.

#10 @peterwilsoncc
5 months ago

  • Milestone changed from 6.7 to 6.8

I've moved this to the next release as RC 1 is approaching and discussion on approach is continuing in the PR.

This ticket was mentioned in PR #7906 on WordPress/wordpress-develop by @poena.


4 months ago
#11

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

Both the name and label properties are required when registering a block style. If the label is missing, assign the name as the value for the label, to ensure that the label is defined.

Testing instructions:
With WP_DEBUG and WP_DEBUG_DISPLAY enabled:

Register an incomplete block style in a theme or plugin file using register_block_style.

register_block_style(
    'core/quote',
    array(
        'name'           => 'test',
        'inline_style' => '.wp-block-quote.is-style-blue-quote { color: blue; }',
    )
);

Open the block editor. Confirm that there is no PHP warning about the label being undefined.

#12 @rinkalpagdar
2 months ago

Test Report

Description

This report validates whether the indicated patch works as expected.

PR tested: https://github.com/WordPress/wordpress-develop/pull/7906

Environment

  • WordPress: 6.8-alpha-59274-src
  • PHP: 8.2.25
  • Server: nginx/1.27.2
  • Database: mysqli (Server: 8.0.40 / Client: mysqlnd 8.2.25)
  • Browser: Chrome 131.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Five 1.0
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with PR.

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


7 weeks ago

@audrasjb commented on PR #7906:


7 weeks ago
#14

## Before patch
I got a warning:

<b>Warning</b>: Undefined array key "label" in <b>/testing-trunk/wp-includes/script-loader.php</b> on line <b>2720</b>

## After patch
No warning 👍

#15 @audrasjb
7 weeks ago

  • Owner set to audrasjb
  • Resolution set to fixed
  • Status changed from new to closed

In 59760:

Editor: Add a fallback to WP_Block_Styles_Registry if the label is missing.

Both name and label properties are required when registering a block style. If the label is missing, assign name as the value for the label, to ensure the property is defined. This avoids a PHP warning in such case.

Props poena, Rahmohn, aaronrobertshaw, audrasjb, rinkalpagdar.
Fixes #52592.

Note: See TracTickets for help on using tickets.