WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#32894 closed defect (bug) (fixed)

Customizer Menus: page enters an infinite reloading loop when using a menu with a walker

Reported by: dennis_f Owned by: westonruter
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description

The problem occurs in the Customize -> Menus section with WP 4.3 Beta 1 - the customized page enters an infinite reloading loop when using a menu with a walker. In order to reproduce it:

  1. The page that is being customized has to load the menu with the walker option, for example:
    wp_nav_menu( array(
            'menu_class'  => 'nav-menu',
            'theme_location' => 'primary',
            'walker' => new Custom_Walker_Nav_Menu
    ) );
    
  1. Following the code from the example above, before editing the menu, assign it to the primary theme location , so that the menu is loaded on the page that is being customized
  1. Reorder the menu items and the page should enter an infinite reloading loop

The issue happens regardless of the code of the custom walker class - I have tested it with an empty class and also with the code from the example here: https://codex.wordpress.org/Function_Reference/wp_nav_menu#Using_a_custom_Walker_class

I have also tested this with different themes and no plugins installed, it happens with the TwentyFifteen theme, when I modify the menu code in the sidebar.php file.

Attachments (3)

twentyfifteen.zip (522.5 KB) - added by dennis_f 5 years ago.
TwentyFifteen theme using a menu walker in the sidebar.php file
32894.diff (2.2 KB) - added by westonruter 5 years ago.
https://github.com/xwp/wordpress-develop/pull/99
32894-with-32781.diff (4.4 KB) - added by westonruter 5 years ago.

Download all attachments as: .zip

Change History (19)

#1 @westonruter
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.3
  • Owner set to westonruter
  • Status changed from new to accepted

#2 follow-up: @westonruter
5 years ago

  • Keywords reporter-feedback added; needs-patch removed

I cannot reproduce the problem here, neither on 4.3 beta 1 or on trunk. I used that themeslug_walker_nav_menu class from the codex page, putting it in functions.php for Twenty Fifteen, and then modified the primary nav menu to be embedded via:

        <?php
                // Primary navigation menu.
                wp_nav_menu( array(
                        'menu_class'     => 'nav-menu',
                        'theme_location' => 'primary',
                        'walker' => new themeslug_walker_nav_menu(),
                ) );
        ?>

But upon changing a nav menu item by reordering or changing a property, the preview does a full refresh as expected in that no apparent infinite loop is experienced.

Note I did find one unexpected thing, however: the custom walker is dropped when the menu is previewed in the preview. This is due to a logic problem in WP_Customize_Nav_Menus::filter_wp_nav_menu_args(). I've re-opened #32781 with a patch to fix that issue.

But otherwise, I can't reproduce the infinite loop problem. Please advise.

#3 in reply to: ↑ 2 @dennis_f
5 years ago

Replying to westonruter:

I cannot reproduce the problem here, neither on 4.3 beta 1 or on trunk [...]

That's strange, I created a second installation on my localhost to test that and the same problem happened. I've been using the Beta Tester plugin that has updated the installation to 4.3-beta1-33088.

I've been trying to trace the problem, so far I found that in the customize-preview-nav-menus.js file, when the refreshMenuInstance() function is called and the walker option is set, the container element:
container = $( '#partial-refresh-menu-container-' + String( instanceNumber ) );
doesn't exist (while it exists without the walker option set)
and then the following code is executed

if ( ! instance.can_partial_refresh || 0 === container.length ) {
	api.preview.send( 'refresh' );
	return;
}

which might be related with the looping issue.

I hope that someone else can reproduce the issue, but meanwhile I'll keep on digging - I'll keep you posted.

#4 @nicholas_io
5 years ago

I had a similar issue, but without using Custom Walker. I was unable to trace the steps to reproduce the problem, sometime it does and sometimes not.

I will do more tests here and try to find something.

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


5 years ago

@dennis_f
5 years ago

TwentyFifteen theme using a menu walker in the sidebar.php file

#6 @dennis_f
5 years ago

Unfortunately I couldn't find what was causing the issue - I tried to trace the JS code, but since I wasn't familiar with it, I got lost between all of the fired events and couldn't find the one that was initializing the looping issue.

I found that what I mentioned above (the non-existing container element) was happening because the can_partial_refresh option is by default set to false when a walker is used (in the filter_wp_nav_menu_args() function of the wp-includes/class-wp-customize-nav-menus.php file) , but I guess that's how it's meant to be.

I also tested this on two more installations - I created another new installation on my localhost and also tried on another installation on another computer - the same issue happened every time.
I have attached the modified TwentyFifteen theme that I have been using to test - it has the walker option set in the sidebar.php file and also contains a walker class in the functions.php file, so if you want you can give it a try.
If you test it, please make sure that the menu that's being edited is assigned as a primary menu so that it is displayed on the page, as the issue doesn't happen when the menu is not assigned.

If you want me to provide any additional info or try some additional testing on my installation, please let me know - I'll be happy to help.

#7 @westonruter
5 years ago

I was able to reproduce the problem now. It looks like the problem is rooted in wp.customize.menusPreview.refreshMenuInstanceDebounced(). When adding a new nav menu item, the type_label property is set whereas in the Customizer preview the settings get exported without this property. The result is that when the preview first loads up and the Customizer re-syncs the settings into the Preview, it detects a change and then triggers an update. Still working on a fix.

#8 @westonruter
5 years ago

  • Keywords has-patch needs-testing added; reporter-feedback removed

OK, please check out 32894.diff. It defers the listening to nav menu setting changes until active, that is until after the Customizer has synced settings into the preview. This ensures that any differences between the JS and PHP representations of the settings won't cause an infinite refresh.

#9 follow-ups: @dennis_f
5 years ago

I have just tested 32894.diff - it works well for me, the infinite refreshing is not happening anymore.
However the walker is not applied to the menus in the customizer (it is only applied on the front-end), is this a known issue?

#10 in reply to: ↑ 9 @celloexpressions
5 years ago

Replying to dennis_f:

I have just tested 32894.diff - it works well for me, the infinite refreshing is not happening anymore.
However the walker is not applied to the menus in the customizer (it is only applied on the front-end), is this a known issue?

The Customizer doesn't support the nav menu walker setup that the admin screen has for a variety of reasons. Instead, we're adding hooks that make the system easier to extend and make cross-compatible. See #32832.

#11 in reply to: ↑ 9 ; follow-up: @westonruter
5 years ago

  • Keywords needs-testing removed

Replying to dennis_f:

However the walker is not applied to the menus in the customizer (it is only applied on the front-end), is this a known issue?

Yes, this is being addressed in #32781. Please test with 32894-with-32781.diff which combines the patch from this ticket with the patch from that ticket.

#12 in reply to: ↑ 11 @dennis_f
5 years ago

Replying to westonruter:

Replying to dennis_f:

However the walker is not applied to the menus in the customizer (it is only applied on the front-end), is this a known issue?

Yes, this is being addressed in #32781. Please test with 32894-with-32781.diff which combines the patch from this ticket with the patch from that ticket.

That works well for me too - with those fixes, the walker code works in the preview and the page refreshes normally, just once.
I just couldn't run the tests changes from the tests/phpunit/tests/customize/nav-menus.php file as I don't have the tests on my setup.

#13 @westonruter
5 years ago

  • Keywords commit added

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


5 years ago

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


5 years ago

#16 @westonruter
5 years ago

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

In 33134:

Customizer: Defer listening to nav menu setting changes until active.

Defer the preview starting to listen to setting changes until after the Customizer has synced settings into the preview. This ensures that any differences between the JS and PHP representations of the settings won't cause an infinite refresh.

See #32911.
Fixes #32894.

Note: See TracTickets for help on using tickets.