Customizer: Remove Menus panel when a theme does not support menus by noisysocks · Pull Request #2217 · WordPress/wordpress-develop

noisysocks · 2022-01-24T02:20:35Z

By having WP_Customize_Panel::check_capabilities(), we can ensure that the Menus panel is removed if a theme does not have support for 'menus' nor 'widgets'. (We show the Menus panel when there is support for 'widgets' so that one can use the Menu widget.)

This ensures that the Menus panel does not appear when using a block theme, which is pretty confusing, as the Navigation block is intended to be the sole place for editing a site's navigation when using a block theme.

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

By overriding check_capabilities(), we can ensure that the Menus panel is removed if a theme does not have support for 'menus' nor 'widgets'. This ensures that the Menus panel does not appear when using a block theme, which is confusing to users.

noisysocks · 2022-01-24T02:21:30Z

src/wp-includes/class-wp-customize-widgets.php

 				'priority'                 => 110,
 				'active_callback'          => array( $this, 'is_panel_active' ),
 				'auto_expand_sole_section' => true,
+				'theme_supports'           => 'widgets',


This hides the Widgets panel if the theme doesn't support 'widgets'. It isn't necessary as it will be hidden anyway because there won't be any registered sidebars. But I thought it would be nice to explicitly add this for consistency with the Menus panel.

I'd recommend against this if it's not needed. Avoids unneded code for either 5.9 or 5.9.1.

Consistency makes sense here. The Core commit should reference menus and widgets per the PR's description, making code archaeology easier.

Edit: Timing. Defer to you and Peter to discuss unneeded code vs consistency.

Yeah, cool. I was on the fence. I've removed the change.

noisysocks · 2022-01-24T02:21:57Z

src/wp-includes/class-wp-customize-panel.php

 	 * @return bool False if theme doesn't support the panel or the user doesn't have the capability.
 	 */
-	final public function check_capabilities() {
+	public function check_capabilities() {


Had to mark this as non final which I think makes sense to do anyway.

I think this makes sense too. It allows panels to manage their own capabilities checks without needing something like this every time:

if ( current_theme_supports( 'some_feature' ) ) { // Add my custom panel. } if ( current_theme_supports( 'some_other_feature' ) ) { // Add my other custom panel. }

This might need a dev note to let extenders know that this is now available to them. What do you think?

Had to mark this as non final which I think makes sense to do anyway.

@westonruter Do you recall if there was an essential reason WP_Customize_Panel::check_capabilities() was marked final?

I don't see any discussion about final in the original tickets that added this code, which makes me think it was just done in an attempt to avoid backwards compatibility issues while this new API was being iterated on. It's a stable API now (committed 7 years ago) so I don't think we need to worry about that anymore.

https://core.trac.wordpress.org/ticket/29197

An alternative is that we could add a $theme_supports_callback property which would work similarly to the existing $active_callback that is used by active() which is marked final. I'm not such a fan of this though because then it isn't clear what should happen when both $theme_supports and $theme_supports_callback is set. Allowing check_capabilities() to be overridden feels simpler and more in line with the Customizer's OOP design to me.

This might need a dev note to let extenders know that this is now available to them. What do you think?

I think it's okay because there was already $theme_supports which allows just this. Overriding check_capabilities() is for more complex use-cases.

Maybe at most a little note in the "miscellaneous API changes" dev note is okay.

Yeah, I went looking for a hint in Slack and IRC but couldn't find anything either.

An alternative may be to allow this->capability to be string|string[] and looping through.

A little more on the history of the final keyword here:

The final keyword was added to WP_Customize_Section and WP_Customize_Setting in changeset 20248.

The only direct reference to check_capabilities() is:

prepare_controls() now removes any settings and sections that return false for check_capabilities().

Then, in changeset 29487, WP_Customize_Panel was separated to its own file. Having previously extended WP_Customize_Section, the change included copying the check_capabilities() method into WP_Customize_Panel.

This was likely just a copy-paste for consistency.

An alternative may be to allow this->capability to be string|string[] and looping through.

Unfortunately we're already using string[] for additional arguments that should be passed into current_theme_supports().

peterwilsoncc

Code LGTM with a CS nit.

I see @costdev has testing notes so haven't pulled it down.

src/wp-includes/customize/class-wp-customize-nav-menus-panel.php

peterwilsoncc · 2022-01-24T02:50:17Z

src/wp-includes/class-wp-customize-widgets.php

 				'priority'                 => 110,
 				'active_callback'          => array( $this, 'is_panel_active' ),
 				'auto_expand_sole_section' => true,
+				'theme_supports'           => 'widgets',


I'd recommend against this if it's not needed. Avoids unneded code for either 5.9 or 5.9.1.

costdev

Looks good and tests well. 👍

Just a change to @since and question about a possible dev note.

costdev · 2022-01-24T02:59:32Z

src/wp-includes/class-wp-customize-panel.php

 	 * @return bool False if theme doesn't support the panel or the user doesn't have the capability.
 	 */
-	final public function check_capabilities() {
+	public function check_capabilities() {


I think this makes sense too. It allows panels to manage their own capabilities checks without needing something like this every time:

if ( current_theme_supports( 'some_feature' ) ) { // Add my custom panel. } if ( current_theme_supports( 'some_other_feature' ) ) { // Add my other custom panel. }

This might need a dev note to let extenders know that this is now available to them. What do you think?

costdev · 2022-01-24T03:04:37Z

src/wp-includes/class-wp-customize-widgets.php

 				'priority'                 => 110,
 				'active_callback'          => array( $this, 'is_panel_active' ),
 				'auto_expand_sole_section' => true,
+				'theme_supports'           => 'widgets',


Consistency makes sense here. The Core commit should reference menus and widgets per the PR's description, making code archaeology easier.

Edit: Timing. Defer to you and Peter to discuss unneeded code vs consistency.

src/wp-includes/customize/class-wp-customize-nav-menus-panel.php

Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>

Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>

noisysocks · 2022-01-24T03:32:59Z

Edit: Timing. Defer to you and Peter to discuss unneeded code vs consistency.

😅 I want the consistency but, yeah, safest to not fix what isn't broken while in RC. I'll open a seperate patch which adds it and we can commit that to trunk only.

noisysocks · 2022-01-24T03:49:17Z

Thanks for reviewing and testing. Feedback addressed. I can commit to both branches today if two devs approve 😀

peterwilsoncc

I can't see an alternative to removing the final, so let's do that.

noisysocks · 2022-01-24T05:39:50Z

Committed in https://core.trac.wordpress.org/changeset/52621 and https://core.trac.wordpress.org/changeset/52623.

westonruter · 2022-01-24T17:11:29Z

src/wp-includes/customize/class-wp-customize-nav-menus-panel.php

+	 *
+	 * @return bool False if theme doesn't support the panel or the user doesn't have the capability.
+	 */
+	public function check_capabilities() {


Another way to do this would have been to implement an active_callback which would return the result of current_theme_supports( 'menus' ) || current_theme_supports( 'widgets' ). The constructor could have:

$this->active_callback = function () { return current_theme_supports( 'menus' ) || current_theme_supports( 'widgets' ); };

The end result would be largely the same, however.

Turns out that the way this was implemented doesn't work as intended. A PHP notice is triggered. See https://core.trac.wordpress.org/ticket/54905#comment:36

noisysocks commented Jan 24, 2022

View reviewed changes

Add @SInCE comment

939e103

peterwilsoncc reviewed Jan 24, 2022

View reviewed changes

Remove unnecessary 'theme_supports' on widgets panel

8e18972

costdev requested changes Jan 24, 2022

View reviewed changes

noisysocks and others added 3 commits January 24, 2022 14:24

Use multiline comment

09fb329

Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>

Set @SInCE to 5.9.0

1d28033

Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>

Remove @peterwilsoncc's troll whitespace

2adb8a5

noisysocks mentioned this pull request Jan 24, 2022

Customizer: Remove Widgets panel if current theme doesn't support 'widgets' #2218

Closed

costdev approved these changes Jan 24, 2022

View reviewed changes

peterwilsoncc approved these changes Jan 24, 2022

View reviewed changes

noisysocks closed this Jan 24, 2022

noisysocks deleted the fix/hide-menus-panel-on-block-themes branch January 24, 2022 05:39

westonruter reviewed Jan 24, 2022

View reviewed changes

westonruter mentioned this pull request Sep 11, 2022

Prevent PHP notice in Customizer when using block theme #3230

Closed

Conversation

noisysocks commented Jan 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costdev Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noisysocks Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noisysocks Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterwilsoncc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costdev Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

noisysocks commented Jan 24, 2022

Uh oh!

noisysocks commented Jan 24, 2022

Uh oh!

peterwilsoncc left a comment

Choose a reason for hiding this comment

Uh oh!

noisysocks commented Jan 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

costdev Jan 24, 2022 •

edited

Loading

noisysocks Jan 24, 2022 •

edited

Loading

noisysocks Jan 24, 2022 •

edited

Loading

costdev Jan 24, 2022 •

edited

Loading