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

Ensure consistent return type in WP_Navigation_Block_Renderer::get_markup_for_inner_block() #59820

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

afragen
Copy link
Member

@afragen afragen commented Mar 13, 2024

Fixes get_markup_for_inner_block()

What?

moves return outside of conditional so method can return '' instead of null.

Why?

Fixes #59814 and https://core.trac.wordpress.org/ticket/60762

How?

move return outside of conditional

Testing Instructions

An empty inner_block on a site triggers this error

Testing Instructions for Keyboard

Screenshots or screencast

Fixes get_markup_for_inner_block()
@afragen afragen requested a review from ajitbohra as a code owner March 13, 2024 10:38
Copy link

github-actions bot commented Mar 13, 2024

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: afragen <[email protected]>
Co-authored-by: swissspidy <[email protected]>
Co-authored-by: getdave <[email protected]>

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

@swissspidy swissspidy changed the title Fix for #59814 Ensure consistent return type in WP_Navigation_Block_Renderer::get_markup_for_inner_block() Mar 13, 2024
@swissspidy swissspidy requested a review from getdave March 13, 2024 20:34
@afragen
Copy link
Member Author

afragen commented Mar 13, 2024

$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 swissspidy added the [Type] Bug An existing feature does not function as intended label Mar 13, 2024
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be in Gutenberg repo.

Update: all good. I saw this, got confused, and scrambled to avoid a commit going into the wrong repo. Turns out it is in the right repo 😄

Screen Shot 2024-03-14 at 09 10 02

@getdave getdave dismissed their stale review March 14, 2024 09:09

My mistake !

@getdave getdave merged commit 87d3d4f into WordPress:trunk Mar 14, 2024
58 of 59 checks passed
@github-actions github-actions bot added this to the Gutenberg 18.0 milestone Mar 14, 2024
@getdave getdave added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Mar 14, 2024
@getdave
Copy link
Contributor

getdave commented Mar 14, 2024

This bug was identified during the RC period for WP 6.5. It represents potential for an error in the Navigation block and therefore should be included in WP 6.5.

@afragen afragen deleted the patch-1 branch March 14, 2024 14:02
youknowriad pushed a commit that referenced this pull request Mar 18, 2024
…arkup_for_inner_block()` (#59820)

* Fix for #59814

Fixes get_markup_for_inner_block()

* fix WPCS issues
@youknowriad
Copy link
Contributor

I just cherry-picked this PR to the update/packages-rc2-6.5 branch to get it included in the next release: ad56431

@youknowriad youknowriad added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Mar 18, 2024
getdave pushed a commit that referenced this pull request Mar 18, 2024
…arkup_for_inner_block()` (#59820)

* Fix for #59814

Fixes get_markup_for_inner_block()

* fix WPCS issues
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Mar 27, 2024
…arkup_for_inner_block()` (WordPress#59820)

* Fix for WordPress#59814

Fixes get_markup_for_inner_block()

* fix WPCS issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nav block renderer error with markup for inner blocks
4 participants