WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 6 months ago

#43113 closed defect (bug) (fixed)

Multiple custom item classes are returned as single string when using 'nav_menu_link_attributes' filter with Customizer preview

Reported by: lrdn Owned by: noisysocks
Milestone: 5.6 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

Normally the variable $item->classes used in a nav_menu_link_attributes filter function returns an array of all classes associated with an item, including custom classes defined in the menu settings. However, when using the Customizer to edit an item all custom classes will be returned as a single string. Only after the changes have been published and the page reloaded will it return the expected array.

I've attached a patch that fixes the problem but I am unsure what actually causes the bug.

Attachments (5)

class-wp-customize-nav-menu-item-setting.diff (579 bytes) - added by lrdn 3 years ago.
43113.diff (1.9 KB) - added by dlh 18 months ago.
43113.2.diff (1.7 KB) - added by noisysocks 6 months ago.
43113.3.diff (1.7 KB) - added by noisysocks 6 months ago.
43113.4.diff (1.7 KB) - added by noisysocks 6 months ago.

Download all attachments as: .zip

Change History (17)

#1 @westonruter
3 years ago

Do you have an example of some code causes the bug to appear?

#2 @lrdn
3 years ago

Weston, thanks for taking a look. Simply add a nav_menu_link_attributes filter function and log the classes array of the item object. I am using the default logger in my example.

<?php

function custom_link_attributes($wp_attributes, $wp_item, $wp_args)
{
        error_log(json_encode($wp_item->classes, JSON_PRETTY_PRINT));
        return $wp_attributes;
}
add_filter('nav_menu_link_attributes', 'custom_link_attributes', 10, 3);

Now edit an existing menu item using the Customizer and define at least 2 custom CSS classes. The log output will show both classes located within the same array item.

[
    "custom-class-1 custom-class-2",
    "menu-item",
    ...
]

As soon as the changes are published, or whenever the menu items are modified in the appearance settings, the item classes will be located in separate array items as expected.

[
    "custom-class-1",
    "custom-class-2",
    "menu-item",
    ...
]

My patch is the earliest point at which I could locate and correct the malformed data, but I am guessing the cause is actually located where the string is handed over by the JavaScript request.

@dlh
18 months ago

#3 follow-up: @dlh
18 months ago

  • Keywords has-patch has-unit-tests needs-testing added
  • Milestone changed from Awaiting Review to 5.4
  • Version changed from 4.9.2 to 4.3

Storing the classes as a string in the setting value seems right to me because wp_update_nav_menu_item() expects to receive a string for menu-item-classes.

The problem occurs (I think) when the nav menu item's properties are overwritten with the customized state in \WP_Customize_Nav_Menu_Item_Setting::filter_wp_get_nav_menu_items(). The string setting value for classes overwrites the array value set by wp_setup_nav_menu_item().

43113.diff attempts to fix this in \WP_Customize_Nav_Menu_Item_Setting::value_as_wp_post_nav_menu_item() by ensuring that a string classes is exploded into an array before it's used to overwrite the property.

@lrdn, would you be able to refresh your memory about this issue and test the patch?

Marking this as a potential small fix to get into 5.4 if it can be tested.

#4 @audrasjb
15 months ago

  • Milestone changed from 5.4 to Future Release

Hi,

With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

This ticket was mentioned in Slack in #core-customize by dlh. View the logs.


6 months ago

#6 in reply to: ↑ 3 @noisysocks
6 months ago

  • Keywords needs-refresh added
  • Owner set to noisysocks
  • Status changed from new to reviewing

Replying to dlh:

Storing the classes as a string in the setting value seems right to me because wp_update_nav_menu_item() expects to receive a string for menu-item-classes.

Agreed.

The problem occurs (I think) when the nav menu item's properties are overwritten with the customized state in \WP_Customize_Nav_Menu_Item_Setting::filter_wp_get_nav_menu_items(). The string setting value for classes overwrites the array value set by wp_setup_nav_menu_item().

Yes, that seems right.

wp_update_nav_menu_item() is ordinarily what will turn the classes string on the menu item object into an array.

During preview, however, the menu item object is set up by WP_Customize_Nav_Menu_Item_Setting::preview()WP_Customize_Nav_Menu_Item_Setting::filter_wp_get_nav_menu_items() which bypasses wp_update_nav_menu_item(). Assumedly this is so that no writes to the database are made.

43113.diff attempts to fix this in \WP_Customize_Nav_Menu_Item_Setting::value_as_wp_post_nav_menu_item() by ensuring that a string classes is exploded into an array before it's used to overwrite the property.

43113.diff no longer applies cleanly. Also, I think we should use ! is_array() instead of is_string() on the very off chance that $classes is a number or something silly.

@noisysocks
6 months ago

@noisysocks
6 months ago

#7 @noisysocks
6 months ago

  • Keywords commit added; needs-testing needs-refresh removed

I attached:

Both patches fix the bug locally and LGTM.

#8 @dlh
6 months ago

Thanks for testing and for refreshing the patch, @noisysocks!

Re. is_string(): Fair enough, although protection against warnings (PHP < 8) or fatal errors (PHP 8) will be lost on the (just-as-silly) chance that $classes is another non-scalar type. Switching from ! is_array() to is_scalar() might be the way to go to handle both cases.

That nitpick aside, this looks good to me too!

#9 @dlh
6 months ago

  • Milestone changed from Future Release to 5.6

Adding this to the 5.6 milestone for consideration as it's a bug fix, but I realize that we're far into betas now and so might need to punt to 5.7.

@noisysocks
6 months ago

#10 @noisysocks
6 months ago

Thanks @dlh. Switched to is_scalar() in 43113.4.diff.

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


6 months ago

#12 @noisysocks
6 months ago

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

In 49624:

Customize: Ensure multiple CSS classes are passed to nav_menu_link_attributes as an array

When multiple CSS classes are added to a menu item, the nav_menu_link_attributes
filter should be called with $item->classes set to an array of CSS class names.

When previewing in the Customizer, however, a single string was being passed to
$item->classes because WP_Customize_Nav_Menu_Item_Setting::preview() bypasses
wp_update_nav_menu_item() and instead uses filter_wp_get_nav_menu_items().

The fix is to make filter_wp_get_nav_menu_items() match what
wp_update_nav_menu_item() does and split the string into an array.

Fixes #43113.
Props dlh.

Note: See TracTickets for help on using tickets.