-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Improve error handling when converting classic to block menus #4858
Conversation
Thank you for this PR and catching those Issues. Looks like the unit tests will need updating to conform to the changes. Do you think you will be able (have capacity) to do that? Also cc'ing @spacedmonkey as he may need to oversee this being merged into Core. |
This will also resolve WordPress/gutenberg#52481 |
Yep, it's on my list to update in the next week or so, I hope. |
@getdave Test updated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes proposed in this PR fix discrepancies in how errors are handled in different situations.
LGTM (but please see #4858 (review)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be changed, @dlh01.
@@ -34,7 +34,7 @@ public static function convert( $menu ) { | |||
$menu_items = wp_get_nav_menu_items( $menu->term_id, array( 'update_post_term_cache' => false ) ); | |||
|
|||
if ( empty( $menu_items ) ) { | |||
return array(); | |||
return ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a BC (backward compat) issue?
tl;dr I think no.
Why?
The method shipped in 6.3.0 is clearly documented as returning a string
, not an array
. Returning an array
then is a bug that needs to be fixed.
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Should be commit ready
Committed via https://core.trac.wordpress.org/changeset/56422. Thank you everyone for your contributions! |
Trac ticket: https://core.trac.wordpress.org/ticket/58823