Make WordPress Core

Opened 2 years ago

Closed 20 months ago

#57169 closed defect (bug) (fixed)

Prevent saving of invalid menu_item_parent

Reported by: azaozz's profile azaozz Owned by: azaozz's profile azaozz
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.1
Component: Menus Keywords: has-patch fixed-major
Focuses: Cc:

Description

Follow up to #56926.

There seems to be a bug in the Customizer that may set $menu_item->menu_item_parent to be the same as $menu_item->ID. Steps to reproduce: https://core.trac.wordpress.org/ticket/56926#comment:21.

Seems there should be some validation of the menu_item_parent before it is saved in the _menu_item_menu_item_parent meta.

Change History (21)

This ticket was mentioned in PR #3663 on WordPress/wordpress-develop by @azaozz.


2 years ago
#1

  • Keywords has-patch added

@azaozz commented on PR #3663:


2 years ago
#2

This should reset the invalid _menu_item_menu_item_parent meta whenever the menu is saved. It also contains the runtime fix as mentioned in https://github.com/WordPress/wordpress-develop/pull/3649#issuecomment-1322728338.

Ideally this should go in after https://github.com/WordPress/wordpress-develop/pull/3649 (https://core.trac.wordpress.org/ticket/56926).

#3 @peterwilsoncc
2 years ago

I think this is good to commit -- I added a nitpick to the PR to better meet the documentation standards but otherwise consider it approved.

#4 @azaozz
2 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 54973:

Menus: Reset menu_item_parent to 0 when the parent is set to the item itself.

Props: peterwilsoncc, SergeyBiryukov, azaozz.
Fixes #57169.

#5 @azaozz
2 years ago

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

Reopen for 6.1.2.

#6 @Chouby
2 years ago

@azaozz I'm afraid that [54973] introduces a breaking change, more specifically the cast to int of the menu item ids at this line.

This is probably not an issue for WordPress itself but will break with some plugins. It's at least the case in Polylang. Indeed, in this plugin, we are dynamically creating menu items with non-numeric ids. I would not be surprised that other multilingual plugins like WPML do the same.

For the language switcher, there is a unique menu item in database (and a unique menu item displayed in the backend). However, this menu item is splitted on frontend to get one menu item per language. Of course, we had to set a unique ID for the dynamic menu items and so we append the language code to the db menu item id resulting in non numeric menu item IDs. If you are interested in the Polylang code: See https://github.com/polylang/polylang/blob/3.3/frontend/frontend-nav-menu.php#L134

Would it be possible to avoid the cast to int in the test?

Last edited 2 years ago by Chouby (previous) (diff)

#7 @Chrystl
2 years ago

Hi there, I'm Christelle from the Polylang team.

=> I would not be surprised that other multilingual plugins like WPML do the same.

Yes, I confirm that WordPress 6.2-alpha breaks also the language switcher of WPML when it is set as dropdown.

#8 @azaozz
2 years ago

  • Keywords fixed-major removed

@Chouby, @Chrystl thanks for reporting this.

we had to set a unique ID for the dynamic menu items and so we append the language code to the db menu item id

I understand. Added a fix to the PR. Could you confirm it fixes this.

#9 follow-up: @manooweb
2 years ago

@azaozz Thanks for your responsiveness.

Sorry but I previously answered too quickly.

<?php
$menu_item_id = intval( $menu_item->ID );

Isn't good in our case (Polylang) because menu_item->ID is really string with non numeric value as "9-en" for example.

So if menu item ("9-en") has its menu item parent 9 (real case), the new condition you introduce is true and you override menu_item_parent property with 0. It causes the bug in our language switcher used in navigation menu.

You really need to compare menu item ID as a string to the menu item parent id.

Last edited 2 years ago by manooweb (previous) (diff)

#10 in reply to: ↑ 9 @azaozz
2 years ago

Replying to manooweb:

menu_item->ID is really string with non numeric value as "9-en" for example.

So if menu item ("9-en") has its menu item parent 9 (real case), the new condition you introduce is true and you override menu_item_parent property with 0.

Right, that was my intention. It prevents a menu item to be set as its own parent (i.e. may cause infinite loop/fatal error).

Looking at Polylang (https://github.com/polylang/polylang/blob/3.3/frontend/frontend-nav-menu.php#L134), the language code is appended to $menu_item->ID and it becomes a string, but $menu_item->menu_item_parent is not changed/remains numeric. So using intval() reverts the $menu_item->ID back to being numeric and it can be compared to menu_item_parent.

I may be missing something but thinking this is the desired behaviour.

Edit: ah, scratch that, think I see what's going on. When appending the language codes Polylang also may overwrite $menu_item->menu_item_parent to be the same as the initial $menu_item->ID. In that case comparing would be best as strings.

Last edited 2 years ago by azaozz (previous) (diff)

#11 @azaozz
2 years ago

@manooweb Updated the PR. Please confirm it works as expected now. Also test what happens when a menu item ID (before appending the locale string) is the same as menu_item_parent meta, if possible.

Last edited 2 years ago by azaozz (previous) (diff)

#12 @manooweb
2 years ago

@azaozz Yes! You understood correctly how our language switcher works. As @Chouby explained we create menu items dynamically from the first menu item (first language) and make their ID unique (used in HTML side) by suffixing the language code to the intitial ID. Their parent menu item id is obviously set to the id from the initial menu item ID. It allows us to really create a submenu then.

Your fix works however, IMHO, I wonder if just remove int casting isn't sufficient.

Last edited 2 years ago by manooweb (previous) (diff)

#13 @Chrystl
2 years ago

@azaozz, just to let you know that with WordPress 6.2-alpha, the issue is solved when replacing


if ( (int) $menu_item->ID === (int) $menu_item->menu_item_parent ) {

by


if ( (string) $menu_item->ID === (string) $menu_item->menu_item_parent ) {

in /wp-includes/nav-menu-template.php at L202

#14 follow-up: @azaozz
2 years ago

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

In 55059:

Menus: Compare $menu_item->ID and $menu_item->menu_item_parent as strings and avoid moidifying them. Plugins may change the ID to a string.

Props Chouby, peterwilsoncc, Chrystl, manooweb, azaozz.
Fixes #57169.

#15 @azaozz
2 years ago

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

Reopen for 6.1.2.

#16 in reply to: ↑ 14 ; follow-up: @SergeyBiryukov
2 years ago

Replying to azaozz:

To avoid modifying the object, use strval() rather than casting to (string).

Hmm, I might be missing something, but I don't see how this:

if ( strval( $menu_item->ID ) === strval( $menu_item->menu_item_parent ) ) {

would be different from this:

if ( (string) $menu_item->ID === (string) $menu_item->menu_item_parent ) {

In my understanding, casting to a string here should only affect this particular comparison and should not modify the object nor change anything else.

A test script like this:

$test = new stdClass;
$test->id = 123;

var_dump( (string) $test->id ); // Should be a string.
var_dump( $test->id );          // Should be an integer.

Results in:

string(3) "123"
int(123)

It looks like this was discussed on the PR with a remark that strval() is used elsewhere in core, but that's actually not the case since [49108] / #42918, which replaced all instances of strval(), intval(), and floatval() with (string), (int), and (float), respectively, except when used in array_map():

$widget_ids = array_map( 'strval', (array) $widget_ids );

So bringing back strval() here is inconsistent with the rest of core. Could we reconsider using (string) for this comparison? What would that affect in practice?

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#17 @manooweb
2 years ago

As @Chrystl said before, casting both operands in (string) fixes our issue.
However I don't know what it could affect elsewhere in the core

#18 in reply to: ↑ 16 @azaozz
2 years ago

Replying to SergeyBiryukov:

$test = new stdClass;
$test->id = 123;

var_dump( (string) $test->id ); // Should be a string.
var_dump( $test->id );          // Should be an integer.

Results in:

string(3) "123"
int(123)

I see... Is that the case in all PHP versions? Was with the impression that casting to (int) was breaking the above plugin because it was removing the modification, but maybe it was just removing the parent as the modified ID is starting with a number, and casting such string to int would keep the number (yea, a bit weird).

...except when used in array_map()

Yep, you're right. When checking I just did a quick grep and saw about 10 other uses.

So bringing back strval() here is inconsistent with the rest of core.

Ah, so WordPress does forbid use of some PHP functions in some cases... As casting doesn't seem to change object properties, all three ways to temporarily convert an int to a string: strval(), (string), and using double quotes appear to work in exactly the same way. It seems it is a question of readability and perhaps a personal preference which one is used. I don't really have any preferences here so feel free to change as you see fit.

Last edited 2 years ago by azaozz (previous) (diff)

#19 @SergeyBiryukov
22 months ago

In 55352:

Coding Standards: Replace strval() with (string) type casting in wp_nav_menu().

This adjusts a newly introduced instance for consistency with the rest of core.

Follow-up to [49108], [55059].

See #57169.

#20 @SergeyBiryukov
20 months ago

  • Milestone changed from 6.1.2 to 6.2.1

Moving to 6.2.1, as there are no plans for 6.1.2 at this time.

#21 @SergeyBiryukov
20 months ago

  • Milestone changed from 6.2.1 to 6.2
  • Resolution set to fixed
  • Status changed from reopened to closed

This is already included in the 6.2 branch as of [55504], no backport needed.

Note: See TracTickets for help on using tickets.