Make WordPress Core

Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#58823 closed defect (bug) (fixed)

Editor: Incorrect error handling when converting classic to block menus

Reported by: dlh's profile dlh Owned by: audrasjb's profile audrasjb
Milestone: 6.3.1 Priority: normal
Severity: normal Version: 6.3
Component: Editor Keywords: has-patch has-unit-tests commit fixed-major dev-reviewed
Focuses: Cc:

Description

Originally filed as https://github.com/WordPress/gutenberg/issues/52481:

Some errors that can occur when WP_Navigation_Fallback::create_classic_menu_fallback() calls WP_Classic_To_Block_Menu_Converter::convert() aren't handled as accurately as they could be.

  1. ::convert() can return a WP_Error object, but this error object won't be detected by the empty() check in ::create_classic_menu_fallback(). The WP_Error return type is also missing from the docs.
  2. ::convert() returns an array instead of a string when no menu items are returned by wp_get_nav_menu_items().

See here: https://github.com/WordPress/wordpress-develop/blob/be1cb4583abc8d0870506dfeec9aad7f6cf82c7e/src/wp-includes/class-wp-navigation-fallback.php#L105-L110

and here: https://github.com/WordPress/wordpress-develop/blob/be1cb4583abc8d0870506dfeec9aad7f6cf82c7e/src/wp-includes/class-wp-classic-to-block-menu-converter.php#L17-L56

Change History (22)

This ticket was mentioned in PR #4858 on WordPress/wordpress-develop by dlh01.


9 months ago
#1

  • Keywords has-patch added

@get_dave commented on PR #4858:


8 months ago
#2

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.

dlh01 commented on PR #4858:


8 months ago
#4

Yep, it's on my list to update in the next week or so, I hope.

dlh01 commented on PR #4858:


8 months ago
#5

@getdave Test updated!

#6 @get_dave
8 months ago

This PR seems to be ready to land. Are any core contributors available to help merge?

#7 @hellofromTonya
8 months ago

  • Keywords has-unit-tests added
  • Milestone changed from Awaiting Review to 6.3.1
  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Pulling this ticket and patch into 6.3.1.

Code was introduced in 6.3.0 via [56052] / #58557.

#8 @hellofromTonya
8 months ago

  • Keywords commit added

The patch is ready. Prepping the trunk commit.

#9 @hellofromTonya
8 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 56422:

Editor: Fix error handling of converting classic to block menus.

Fixes the error handling for when WP_Classic_To_Block_Menu_Converter::convert() returns an instance of WP_Error. WP_Navigation_Fallback::create_classic_menu_fallback() now checks for is_wp_error() and if true, returns the error. And the @return type is updated to string|WP_Error.

Also includes a fix in the return type in WP_Classic_To_Block_Menu_Converter::convert() to return an empty string instead of an array instead, i.e. when bailing out for no menu items returned by wp_get_nav_menu_items(). The return type is clearly documented as a string.

Follow-up to [56052].

Props dlh, get_dave, antonvlasenko, hellofromTonya.
Fixes #58823.

#10 @hellofromTonya
8 months ago

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to 6.3.x consideration.

Pinging @azaozz.

@hellofromTonya commented on PR #4858:


8 months ago
#11

Committed via https://core.trac.wordpress.org/changeset/56422. Thank you everyone for your contributions!

#12 follow-up: @hellofromTonya
8 months ago

@azaozz I moved this fix into 6.3.1 as it was introduced in 6.3.0. It might not be severe enough to be bundled into the .1 minor and could likely wait until .2 minor. Take a look and see what you think.

#13 in reply to: ↑ 12 @azaozz
8 months ago

Replying to hellofromTonya:

Thanks for the ping! Looking at [56422] it seems simple and straightforward enough to add to 6.3.1. The only thing I'm a bit unsure is whether it affects a lot of people, i.e. if it is a higher priority fix.

#14 @azaozz
8 months ago

  • Keywords fixed-major added

#15 @audrasjb
8 months ago

I don't think it's a high priority issue, but given the minimal scope of the patch I think we can safely include this bugfix in 6.3.1.

#16 @hellofromTonya
8 months ago

  • Owner hellofromTonya deleted
  • Status changed from reopened to reviewing

Removing myself as owner, just in case I'm not available to do the backport to the 6.3-branch, once approved for backport.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


8 months ago

#18 @audrasjb
8 months ago

  • Keywords dev-reviewed added; dev-feedback removed

As per today's bug scrub, marking this as reviewed.

#19 @audrasjb
8 months ago

  • Owner set to audrasjb
  • Status changed from reviewing to accepted

Self assigning for backport

#20 @audrasjb
8 months ago

  • Milestone changed from 6.3.1 to 6.3.2

WP 6.3.1 is going to be released in the next few days, so let's move this ticket to 6.3.2 to give it more time to be committed and backported.

#21 @audrasjb
8 months ago

  • Milestone changed from 6.3.2 to 6.3.1

Reassigning to milestone 6.3.1.

#22 @audrasjb
8 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 56426:

Editor: Fix error handling of converting classic to block menus.

Fixes the error handling for when WP_Classic_To_Block_Menu_Converter::convert() returns an instance of WP_Error.
WP_Navigation_Fallback::create_classic_menu_fallback() now checks for is_wp_error() and if true, returns the error. And the @return type is updated to
string|WP_Error.

Also includes a fix in the return type in WP_Classic_To_Block_Menu_Converter::convert() to return an empty string instead of an array instead, i.e. when bailing
out for no menu items returned by wp_get_nav_menu_items(). The return type is clearly documented as a string.

Follow-up to [56052].

Props dlh, get_dave, antonvlasenko, hellofromTonya.
Reviewed by azaozz, audrasjb.
Merges [56422] to the 6.3 branch.
Fixes #58823.

--Cette ligne, et les suivantes ci-dessous, seront

ignorées--

_M .
M src/wp-includes/class-wp-classic-to-block-menu-converter.php
M src/wp-includes/class-wp-navigation-fallback.php
M tests/phpunit/tests/editor/classic-to-block-menu-converter.php

Last edited 8 months ago by audrasjb (previous) (diff)
Note: See TracTickets for help on using tickets.