Avoid null for inner block markup value, always return string

[draganescu]

What?

Fixes: #59814

Why?

It's a bug.

How?

Did exactly what the issue says to do.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: draganescu <andraganescu@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: afragen <afragen@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[getdave]

Thank you for jumping on this and seeking to resolve so quickly 👍

However, I have concerns about the current state of the change (see comments).

I also think with this type of fix we should write a test to validate the failure and then write code that makes that test go green.

I think we can do that in phpunit/blocks/class-wp-navigation-block-renderer-test.php.

[getdave]

I'm not following this change.

The $inner_block_content "not empty". So we then check whether it needs a wrapper <li>. If it doesn't we return an empty string. I'm not sure that makes sense.

To me we need to ensure that the method returns a string as per the @return. The method should not be returning an empty string just because it doesn't need an <li> wrapper.

Therefore if $inner_block_content is empty we should return early with an empty string, and only allow the remainder of the method to execute if there is content.

[draganescu]

Yea, I only implemented the text in the issue. Will revise

[draganescu]

I did what the issue said would work. The whole function is:

private static function get_markup_for_inner_block( $inner_block ) {
	$inner_block_content = $inner_block->render();
	if ( ! empty( $inner_block_content ) ) {
		if ( static::does_block_need_a_list_item_wrapper( $inner_block ) ) {
			return '<li class="wp-block-navigation-item">' . $inner_block_content . '</li>';
		} else {
			$inner_block_content = '';
		}

		return $inner_block_content;
	}
}

The identified problem is that it sometimes can return null.
Since empty(null) is true then if $inner_block_content is null we won't be in the if in the 1st place.

So the problem is that if there is empty $inner_block_content then the function returns null which is the default return.

[draganescu]

In 8f07a37 I updated to always return an empty string if the rendered content is null or empty.

[getdave]

I did what the issue said would work. The whole function is:

I know you did. However, I wasn't confident in that solution so I felt I needed to raise that in the review.

Since empty(null) is true then if $inner_block_content is null we won't be in the if in the 1st place.

I must admit I'd forgotten it worked like that.

In 8f07a37 I updated to always return an empty string if the rendered content is null or empty.

This is a much better solution.

[getdave]

@afercia as the reporter of the Issue are you able to validate and approve?

[afragen]

I’m curious as to why not use the fix as outlined in #59820

$inner_block_content starts out as an empty string. It shouldn’t need to be set to an empty string before the return. The only reason the current code returns null when the value is empty ('') is that the variable is not returned from inside the conditional. The method simply exits. $inner_block->render() returns ''. All that really needs to happen to return '' is to move the return outside of the conditional.

FWIW, I have tested this locally and it does fix the issue.

[swissspidy]

@getdave The reporter was @afragen, not @afercia

Also, yeah there is already a PR in #59820, so maybe focus on that one?

[draganescu]

@swissspidy thanks for linking to the other PR I assumed it was new and urgent and didn't search. I'll close this one.

[getdave]

@getdave The reporter was @afragen, not @afercia

Also, yeah there is already a PR in #59820, so maybe focus on that one?

My mistake. Thanks for correcting me.