Make WordPress Core

Opened 6 years ago

Closed 21 months ago

#52592 closed defect (bug) (fixed)

PHP warning when the label property is missing from register_block_style

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

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 2 years ago.
Refactoring and adding tests

Download all attachments as: .zip

Change History (16)

#1 @poena
2 years 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
2 years ago

  • Summary PHP notice when the label property is missing from register_block_style → 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.


2 years 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
2 years 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 2 years ago by poena (previous) (diff)

#5 @poena
2 years ago

  • Milestone Awaiting Review → 6.7

#6 @poena
2 years ago

  • Keywords needs-unit-tests added

#7 @poena
2 years 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
2 years 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
2 years ago

Refactoring and adding tests

​@aaronrobertshaw commented on ​PR #7008:


2 years 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
2 years ago

  • Milestone 6.7 → 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.


23 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
21 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.


21 months ago

​@audrasjb commented on ​PR #7906:


21 months 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
21 months ago

  • Owner set to audrasjb
  • Resolution → fixed
  • Status new → 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.