Skip to content
This repository has been archived by the owner on Dec 27, 2022. It is now read-only.

[WIP] Support Customize Menus #50

Closed
wants to merge 16 commits into from

Conversation

valendesigns
Copy link
Contributor

Fixes #2

@valendesigns valendesigns added this to the 0.5.0 milestone Jun 15, 2016
add_filter( 'theme_mod_nav_menu_locations', array( $this, 'filter_theme_mod_nav_menu_locations' ) );
add_filter( 'wp_get_nav_menus', array( $this, 'filter_wp_get_nav_menus' ) );
add_filter( 'wp_get_nav_menu_items', array( $this, 'filter_wp_get_nav_menu_items' ), 10, 3 );
add_filter( 'wp_get_nav_menu_object', array( $this, 'filter_wp_get_nav_menu_object' ), 10, 2 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we not be able to re-use the methods in WP_Customize_Nav_Menus? https://github.com/xwp/wordpress-develop/blob/57d2c46591e6593c8d6bfcb2100b6538e80f9dce/src/wp-includes/customize/class-wp-customize-nav-menu-setting.php#L231-L234

That is, use a similar approach to re-using methods from WP_Customize_Widgets?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually while debugging I saw that these filters are already used via Customize_Snapshot_Manager::preview().
https://github.com/xwp/wp-customize-snapshots/blob/master/php/class-customize-snapshot-manager.php#L862-L887

If the $setting is a menu setting, the filters are set by WP_Customize_Nav_Menu_Setting::preview() but they are not making a difference right now for the Menus section. They do work in case of widgets, e.g. the menus created via snapshot are displayed in the selection list when adding a Custom Menu widget.

I'm right now testing the filters used in WP_Customize_Nav_Menus as suggested to see if there's still something missing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: the issue is that these filters are used only settings-specifically but the menus are initially loaded at this moment in WP_Customize_Nav_Menus:
https://github.com/xwp/wordpress-develop/blob/master/src/wp-includes/class-wp-customize-nav-menus.php#L509

Loading the Customize_Nav_Menu_Setting by itself to use the filters would require being related to existing setting IDs which are unknown before loading the Snapshot data. Any ideas how to still use these instead of creating new filters? Otherwise it seems that new filters are needed.

There's a second issue as well. The following function doesn't return a correct value when used with negative menu ID-s:
https://github.com/xwp/wordpress-develop/blob/57d2c46591e6593c8d6bfcb2100b6538e80f9dce/src/wp-includes/customize/class-wp-customize-nav-menu-section.php#L37-L42
E.g. nav_menu[-123] returns 0.

@zzramesses
Copy link

I tested an existing menu I had on a site. I edited the placement/order of items and sub-items in this menu. I also added items to the menu. This menu wasn't currently set to a location. I set the menu to a registered menu location. All of these actions resulted in the information being saved in the snapshot correctly. I even went into the snapshot and made edits and saw these changes reflected

@valendesigns
Copy link
Contributor Author

Ok, now we need to add back the lost coverage and test with the REST API.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage decreased (-13.2%) to 80.333% when pulling 8ae0764 on bugfix/saving-menus-issue_2 into 082dd2e on master.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage decreased (-9.9%) to 83.661% when pulling 44d8d4a on bugfix/saving-menus-issue_2 into 082dd2e on master.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage decreased (-4.01%) to 89.561% when pulling b7176d3 on bugfix/saving-menus-issue_2 into 082dd2e on master.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage decreased (-1.8%) to 91.781% when pulling 6393ddd on bugfix/saving-menus-issue_2 into 082dd2e on master.

@valendesigns
Copy link
Contributor Author

Closing in favor of #55 which would merge into develop.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants