Opened 2 years ago
Closed 20 months ago
#57169 closed defect (bug) (fixed)
Prevent saving of invalid menu_item_parent
Reported by: | azaozz | Owned by: | 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
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
@
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
@
2 years ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from new to closed
In 54973:
#5
@
2 years ago
- Keywords fixed-major added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopen for 6.1.2.
#6
@
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?
#7
@
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
@
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:
↓ 10
@
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.
#10
in reply to:
↑ 9
@
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.
#11
@
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.
#12
@
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.
#13
@
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:
↓ 16
@
2 years ago
- Resolution set to fixed
- Status changed from reopened to closed
In 55059:
#15
@
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:
↓ 18
@
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?
#17
@
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
@
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. 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.
Trac ticket: https://core.trac.wordpress.org/ticket/57169