Prevent saving of invalid menu_item_parent

[azaozz]

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.

Trac ticket: https://core.trac.wordpress.org/ticket/57169

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).

[peterwilsoncc]

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.

[azaozz]

In 54973:

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

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

[azaozz]

Reopen for 6.1.2.

[Chouby]

@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?

[Chrystl]

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.

[azaozz]

@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.

[manooweb]

@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.

[azaozz]

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. Or does Polylang also append the language code to the $menu_item->menu_item_parent?

[azaozz]

@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.

[manooweb]

@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.

[Chrystl]

@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

[azaozz]

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.

[azaozz]

Reopen for 6.1.2.

[SergeyBiryukov]

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?

[manooweb]

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

[azaozz]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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

[SergeyBiryukov]

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