Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undefined variable notice in render_block_core_search() #16188

Closed
SergeyBiryukov opened this issue Jun 16, 2019 · 3 comments · Fixed by #16189
Closed

Undefined variable notice in render_block_core_search() #16188

SergeyBiryukov opened this issue Jun 16, 2019 · 3 comments · Fixed by #16189
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).

Comments

@SergeyBiryukov
Copy link
Member

Describe the bug
Reported by @sandrowuermli on WordPress Slack.

If you remove the label from the Search block, the page breaks due to a PHP notice:

PHP Notice: Undefined variable: label_markup in wp-includes/blocks/search.php on line 51

To reproduce
Steps to reproduce the behavior:

  1. Replace the core Search block with the one that doesn't have a label:
function my_register_block_core_search() {
	register_block_type(
		'core/search',
		array(
			'attributes'      => array(
				'placeholder' => array(
					'type'    => 'string',
					'default' => '',
				),
				'buttonText'  => array(
					'type'    => 'string',
					'default' => __( 'Search' ),
				),
			),

			'render_callback' => 'render_block_core_search',
		)
	);
}

function my_replace_block_core_search() {
	remove_action( 'init', 'register_block_core_search' );
	add_action( 'init', 'my_register_block_core_search' );
}
add_action( 'plugins_loaded', 'my_replace_block_core_search' );
  1. Go to Edit Post screen.
  2. See the PHP notice:
PHP Notice: Undefined variable: label_markup in wp-includes/blocks/search.php on line 51

The notice happens because the $label_markup variable is defined conditionally. Making sure that the variable is always defined would fix the issue.

Expected behavior
PHP notice should not happen.

Screenshots
Screenshot of the PHP notice

Desktop (please complete the following information):

  • OS: Windows
  • Browser: Microsoft Edge
  • Version: 42.17134.1.0

Additional context

  • Tested on WordPress trunk (5.3-alpha-45282-src).
SergeyBiryukov added a commit that referenced this issue Jun 16, 2019
@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jun 16, 2019
@afercia
Copy link
Contributor

afercia commented Jun 16, 2019

It's important to note that the search input field should always be labelled.

WordPress aims to produce semantic, web standards compliant, and accessible markup in the front end. This block should make sure there's a label. If developers want to visually hide the label (which is not recommended), there should be an option or a fallback to a visually hidden label with screen-reader-text. Worth noting the legacy search widget does print out a visually hidden label.

I'd also suggest to document the importance of providing a label somewhere in the documentation.

@sandrowuermli
Copy link

sandrowuermli commented Jun 16, 2019

@afercia something like this?

if ( ! empty( $attributes['label'] ) ) {
    $label_markup = sprintf(
        '<label for="%s" class="wp-block-search__label">%s</label>',
        $input_id,
        $attributes['label']
    );
} else {
    $label_markup = sprintf(
        '<label for="%s" class="wp-block-search__label screen-reader-text">%s</label>',
        $input_id,
        __('Search')
    );   
}

Maybe you could use the default from register_block_core_search instead of another __('Search') to DRY.
@SergeyBiryukov FYI

@afercia
Copy link
Contributor

afercia commented Jun 16, 2019

@sandrowuermli thanks, yes something like that. Maybe the if/else can be avoided and the CSS class / default label be conditionally passed to the sprintf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants