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.

Sign up for GitHub

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

Jump to bottom

Improve error handling when converting classic to block menus #52482

Closed
wants to merge 1 commit into from
Closed

Improve error handling when converting classic to block menus #52482

wants to merge 1 commit into from

Conversation

dlh01
Copy link
Contributor

@dlh01 dlh01 commented Jul 10, 2023

What?

See #52481. Addresses some errors that can occur when Gutenberg_Navigation_Fallback::create_classic_menu_fallback() calls Gutenberg_Classic_To_Block_Menu_Converter::convert().

Why?

To avoid unexpected behavior or errors resulting from incorrect type handling.

How?

Improving type-checking and returning types consistent with the documentation
@dlh01 dlh01 requested a review from spacedmonkey as a code owner July 10, 2023 18:00
draganescu
draganescu approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

These updates make sense to me 🚀
getdave
getdave approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@getdave getdave left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. Thanks for spotting these.

Will need a complementary backport PR to WP Core for WP 6.3.

@dlh01 Are you in a position to take that on?
@getdave getdave added [Type] Bug An existing feature does not function as intended Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Jul 14, 2023
spacedmonkey
spacedmonkey reviewed Jul 14, 2023
View reviewed changes
lib/compat/wordpress-6.3/class-gutenberg-classic-to-block-menu-converter.php
@@ -31,7 +31,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 '';
Copy link
Member

Choose a reason for hiding this comment

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

Should this return a WP_error object?
Copy link
Contributor

Choose a reason for hiding this comment

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

It's also got me thinking. Any changes here will possible effect the fallback handling code so we'd need to check there.

I'd hope if the tests aren't failing it's all ok but it's entirely possible there isn't 100% coverage 🤔
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I don't think this should be an error. That's because we have converted the menu but it was just empty. That doesn't make it invalid. The conversion was a success.

We're just exiting early to avoid running the "to_blocks" code.

Later on we throw an error if the conversion to blocks fails because that's a true "error".
@dlh01
Copy link
Contributor Author

dlh01 commented Jul 15, 2023

@getdave Sure, I think I can do that. Is there anything more to it than filing a core PR with the same changes?
@getdave
Copy link
Contributor

getdave commented Jul 17, 2023 •
edited
Loading

@getdave Sure, I think I can do that. Is there anything more to it than filing a core PR with the same changes?

That would be great. Thank you.

Yes you raise a Core PR with the same changes in the equivalent files. And you need a Trac ticket.
@dlh01
Copy link
Contributor Author

dlh01 commented Jul 17, 2023

Done in https://core.trac.wordpress.org/ticket/58823 and WordPress/wordpress-develop#4858. Thanks!
@ramonjd ramonjd added the Needs PHP backport Needs PHP backport to Core label Jul 18, 2023
@ramonjd
Copy link
Member

ramonjd commented Jul 18, 2023

Looks like the failing PHP tests are related:

There was 1 failure:

1) Gutenberg_Classic_To_Block_Menu_Converter_Test::test_returns_empty_array_for_menus_with_no_items
Result should be empty array.
Failed asserting that '' is of type "array".

/var/www/html/wp-content/plugins/gutenberg/phpunit/class-gutenberg-classic-to-block-menu-converter-test.php:211
phpvfscomposer:///var/www/html/wp-content/plugins/gutenberg/vendor/phpunit/phpunit/phpunit:106
@tellthemachines tellthemachines removed the Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Aug 8, 2023
@dlh01
Copy link
Contributor Author

dlh01 commented Sep 14, 2023

https://core.trac.wordpress.org/changeset/56422
@dlh01 dlh01 closed this Sep 14, 2023
@SiobhyB SiobhyB removed the Needs PHP backport Needs PHP backport to Core label Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
7 participants
@dlh01 @getdave @ramonjd @draganescu @spacedmonkey @SiobhyB @tellthemachines