Make WordPress Core

Opened 17 months ago

Closed 8 months ago

Last modified 8 months ago

#54905 closed defect (bug) (fixed)

PHP notice while accessing Customizer

Reported by: boniu91's profile Boniu91 Owned by: westonruter's profile westonruter
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Customize Keywords: has-testing-info has-screenshots has-patch dev-feedback commit
Focuses: Cc:

Description

Env

  • WordPress 5.9
  • Chrome 97.0.4692.71
  • Windows 10
  • Theme: Twenty Twenty Two
  • Plugin: WooCommerce

Steps to reproduce

  1. Open Appearance > Customizer
  2. Check the debug log
[25-Jan-2022 20:15:07 UTC] PHP Notice:  Trying to get property 'title' of non-object in /var/www/domain.ovh/htdocs/wp-includes/class-wp-customize-nav-menus.php on line 523
[25-Jan-2022 20:15:07 UTC] PHP Notice:  Trying to get property 'title' of non-object in /var/www/domain.ovh/htdocs/wp-includes/class-wp-customize-nav-menus.php on line 1152

Attachments (2)

54905.diff (3.4 KB) - added by azouamauriac 14 months ago.
Patch updated.
issue-56142.diff (2.9 KB) - added by bobbingwide 11 months ago.
Patch for 56142 - a duplicate of 54905 - to avoid 3 Warning messages from the Customizer for FSE theme

Download all attachments as: .zip

Change History (47)

#1 @pbiron
17 months ago

  • Milestone changed from Awaiting Review to 5.9.1

#2 @jamieblomerus
17 months ago

Have the same problem with 5.9 sometimes, but not always.

Last edited 17 months ago by jamieblomerus (previous) (diff)

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


17 months ago

#4 @pbiron
17 months ago

line 523 is just trying to enqueue uneeded JS and line 1153 is trying to output some HTML that doesn't appear on screen because the Nav Menus panel doesn't.

#5 @pbiron
17 months ago

  • Component changed from General to Customize

This ticket was mentioned in PR #2227 on WordPress/wordpress-develop by costdev.


17 months ago
#6

  • Keywords has-patch added

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


16 months ago

#8 @Boniu91
16 months ago

  • Keywords needs-testing has-testing-info added

mauriac commented on PR #2227:


16 months ago
#9

hi thanks for the patch, tested and it works on my side, just one thought:

on line 1158, I think the if controle should wrap the span node too, to avoid orphan node https://prnt.sc/26tbhbx . thanks.

#10 @SergeyBiryukov
16 months ago

#55111 was marked as a duplicate.

#11 @SergeyBiryukov
16 months ago

#55171 was marked as a duplicate.

#12 @hellofromTonya
16 months ago

  • Milestone changed from 5.9.1 to 5.9.2

As discussions are continuing in the PR, moving this ticket to 5.9.2 to give time to resolve.

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


16 months ago

#14 follow-up: @rafiahmedd
16 months ago

@Boniu91 couldn't reproduce the same issue in my environment. Which version of PHP you are running?

#15 @ironprogrammer
16 months ago

I was able to reproduce the issue. tl;dr, the patch fixes the original notices, but there is a strikingly similar notice now caused by class-wp-customize-widgets.php. @costdev, would you be able to address the same issue there in your PR?

Test Report

PR 2227

Env

  • WordPress 6.0-alpha-52448-src
  • Safari 15.3
  • macOS 12.2.1 (Monterey)
  • Theme: Twenty Twenty-Two
  • Gutenberg DISABLED 🔴
  • Plugin: WooCommerce (activated, but not configured)
  • wp-config: define( 'WP_DEBUG_LOG', true );

Steps to Test (BEFORE PATCH)

  1. Navigate to Appearance > Customize (menu enabled by WooCommerce).
  2. Observe that /wp-content/debug.log contains the following PHP notices (paths modified for privacy):
    [25-Feb-2022 17:53:11 UTC] PHP Notice:  Trying to get property 'title' of non-object in [...]/wp-includes/class-wp-customize-nav-menus.php on line 523
    [25-Feb-2022 17:53:12 UTC] PHP Notice:  Trying to get property 'title' of non-object in [...]/wp-includes/class-wp-customize-widgets.php on line 895
    [25-Feb-2022 17:53:12 UTC] PHP Notice:  Trying to get property 'title' of non-object in [...]/wp-includes/class-wp-customize-nav-menus.php on line 1152
    

Steps to Test (AFTER PATCH )

  1. Navigate to Appearance > Customize (menu enabled by WooCommerce).
  2. Observe that /wp-content/debug.log contains the following PHP notices (paths modified for privacy):
    [25-Feb-2022 18:06:08 UTC] PHP Notice:  Trying to get property 'title' of non-object in [...]/wp-includes/class-wp-customize-widgets.php on line 895
    

Expected Results (✅/❌)

  • Patch addresses originally reported PHP notices. ✅
  • Same notice appears in class-wp-customize-widgets.php in wordpress-develop:trunk. ❌

#16 @costdev
16 months ago

@ironprogrammer Thanks for testing!

I'll update the PR to address the other notice too.

#17 @huskyr
15 months ago

I'm having the same issue without the WooCommerce plugin, with a custom theme. I don't get the issue when switching to one of the standard themes. Is there a workaround without waiting for the upgrade?

#18 follow-up: @ironprogrammer
15 months ago

Hi, @huskyr:

As this ticket addresses a PHP notice, it shouldn't impact site performance or the frontend. If the notice is visible in the admin UI, you could disable WP_DEBUG or WP_DEBUG_DISPLAY in wp-config.php.

I'm not aware of a workaround or fix you could apply to the custom theme, but if you wanted the proposed fix sooner than it ships, then you could apply the patch directly.

#19 @ironprogrammer
15 months ago

  • Keywords has-screenshots added

Adding screenshots that help clarify when these notices would appear. When the menus in question aren't enabled, their underlying markup is still emitted to the page, which triggers the notices.

Screenshots

https://cldup.com/dDsgVHq1mk.png
Classic theme active, with Menus and Widgets support.

https://cldup.com/FvnX8gxkah.png
Block theme active (or classic theme w/o Menus and/or Widgets support), which triggers notice(s).

https://cldup.com/E2f4skQfQR.png
Example DOM footprint of a disabled customize-action with PHP notice (no Customizer widgets support).

#20 in reply to: ↑ 18 @huskyr
15 months ago

Hey ironprogrammer, thanks for the extensive explanation and screenshots! You're right, i don't see the notice on my production server because WP_DEBUG is indeed disabled there.

#21 @audrasjb
15 months ago

  • Milestone changed from 5.9.2 to 5.9.3

Moving to milestone 5.9.3 since we're about to release 5.9.2.

#22 in reply to: ↑ 14 @azouamauriac
15 months ago

Hi there, to reproduce the issue you should follow this step:
WordPress 5.9 hides the Customize menu item, if you are using a Full Site Editing theme, so you need to then access the /wp-admin/customize.php address manually to fully test this out.
as per this slack chat https://wordpress.slack.com/archives/C02RQBWTW/p1645644792576189

Replying to rafiahmedd:

@Boniu91 couldn't reproduce the same issue in my environment. Which version of PHP you are running?

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


15 months ago

#24 @audrasjb
15 months ago

Hello @costdev, are you still working on this PR?

Also @azouamauriac feel free to propose a new patch/PR to address the issue you underlined in the PR :)

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


15 months ago

#26 @audrasjb
15 months ago

  • Milestone changed from 5.9.3 to 5.9.4

This issue still needs a patch and WordPress 5.9.3 RC1 is scheduled tomorrow.
Moving for 5.9.4 consideration.

#27 @azouamauriac
14 months ago

  • Keywords needs-patch added; has-patch removed

Hello everyone, I will update the patch here shortly.

Last edited 14 months ago by azouamauriac (previous) (diff)

@azouamauriac
14 months ago

Patch updated.

#28 @azouamauriac
14 months ago

  • Keywords has-patch added; needs-patch removed

#29 @ironprogrammer
13 months ago

  • Keywords needs-testing removed

Thank you, @azouamauriac, for the updated patch!

Test Report

Patch 54905 👍🏻

Env

  • WordPress 6.1-alpha-53344-src
  • Chrome 100.0.4896.127
  • macOS 12.3.1 (Monterey)
  • Theme: Twenty Twenty-Two
  • Gutenberg DISABLED 🔴
  • Plugin: WooCommerce (activated, but not configured)
  • In wp-config.php: define( 'WP_DEBUG_LOG', true );

Steps to Test (BEFORE PATCH)

  1. Navigate to Appearance > Customize (menu enabled by WooCommerce).
  2. Observe that upon page load, the following PHP notices are logged to /wp-content/debug.log (paths modified for privacy):
[03-May-2022 20:26:43 UTC] PHP Notice:  Trying to get property 'title' of non-object in [...]/wp-includes/class-wp-customize-nav-menus.php on line 523
[03-May-2022 20:26:43 UTC] PHP Notice:  Trying to get property 'title' of non-object in [...]/wp-includes/class-wp-customize-widgets.php on line 899
[03-May-2022 20:26:43 UTC] PHP Notice:  Trying to get property 'title' of non-object in [...]/wp-includes/class-wp-customize-nav-menus.php on line 1152

Steps to Test (AFTER PATCH)

  1. Navigate to Appearance > Customize (menu enabled by WooCommerce).
  2. Observe that on page load, the identified PHP notices are NOT logged to /wp-content/debug.log.

Expected Results (✅)

  • The patch mitigates logging of the identified PHP notices.

#30 follow-up: @audrasjb
13 months ago

  • Milestone changed from 5.9.4 to 6.1

Moving this ticket to next major release since it wasn't addressed during this cycle. Anyone is welcome to move it back to 6.0.x minor releases cycle if a patch is ready to ship.

#31 in reply to: ↑ 30 @azouamauriac
13 months ago

  • Keywords dev-feedback added

Replying to audrasjb:

Moving this ticket to next major release since it wasn't addressed during this cycle. Anyone is welcome to move it back to 6.0.x minor releases cycle if a patch is ready to ship.

This one has a patch though, and test report.

#32 @hellofromTonya
13 months ago

I'm wondering why this happens. Yes, I see the effect where the notification itself is happening due to no object being returned and no guard in place. However, going past this, how does the code even get to this point?

What do I mean?

With block themes, the intent is for the Menus and Widgets sections not be displayed in Customizer. Okay, then thinking more broadly, when should WP_Customize_Widgets and WP_Customize_Nav_Menus either bail out or not be instantiated?

Is the "removal" logic too late in the execution stack?
Is there an earlier approach to prevent unneeded code from executing? Resolving this fixes this ticket and enhances performance.

Off the top of my head, I'm not sure. Deeper dive is needed. Raising it here though for broader thinking and discussion.

#33 @pbiron
13 months ago

@hellofromTonya I haven't had a chance to look further into this since it was originally reported, but here are some thoughts I shared in slack at the time:

I'm not really familiar with most the code for WP_Customize_Nav_Menus but if $this->manager->get_panel( 'nav_menus' ) returns null then it seems like most (if not all) of the hooks added in __constuct() should just be skipped, right?

I'm sure I real fix is a little more complicated than that, but it seems that a deeper dive into how each of the Composer objects initialize themselves is needed.

#34 @dlh
11 months ago

#56142 was marked as a duplicate.

#35 @bobbingwide
11 months ago

My patch for #56142 was different from the one attached here.
It covers an additional Warning message for when Widgets are not supported either.

@bobbingwide
11 months ago

Patch for 56142 - a duplicate of 54905 - to avoid 3 Warning messages from the Customizer for FSE theme

#36 @westonruter
9 months ago

  • Owner set to westonruter
  • Status changed from new to accepted

After a lot of digging, I finally found the cause. The issue was introduced in [52621] where a WP_Customize_Nav_Menus_Panel::check_capabilities() method override was introduced which caused the return to be false if the current theme doesn't support menus nor widgets. Original PR: https://github.com/WordPress/wordpress-develop/pull/2217

The problem is that since check_capabilities returns false, when WP_Customize_Manager::prepare_controls() runs it will remove any registered panels that the user doesn't have the capability to manage.

Ironically, I actually reviewed that PR and I thought the approach would work, but clearly it doesn't. I did provide an alternative solution, however, which should actually fix the issue here: https://github.com/WordPress/wordpress-develop/pull/2217#discussion_r790977534

I'll create a patch for this.

This ticket was mentioned in PR #3230 on WordPress/wordpress-develop by westonruter.


9 months ago
#37

The issue was introduced in https://github.com/WordPress/wordpress-develop/pull/2217 where a WP_Customize_Nav_Menus_Panel::check_capabilities() method override was introduced which caused the return to be false if the current theme doesn't support menus nor widgets.

The problem is that since check_capabilities returns false, when WP_Customize_Manager::prepare_controls() runs it will remove any registered panels that the user doesn't have the capability to manage.

Ironically, I actually reviewed that PR and I thought the approach would work, but clearly it doesn't. I did provide an alternative solution, however, which should actually fix the issue here: https://github.com/WordPress/wordpress-develop/pull/2217#discussion_r790977534

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

#38 @westonruter
9 months ago

@bobbingwide My patch doesn't impact class-wp-customize-widgets.php as yours does. I wasn't able to reproduce the issue with widgets, however. This ticket is specifically regarding menus nonetheless.

noisysocks commented on PR #3230:


9 months ago
#39

Makes sense. Marking the panel as inactive instead of removing it would indeed make it so that calls to get_panel( 'nav_menus' ) don't return an empty value.

What I'm wondering though is: why do we even run WP_Customize_Nav_Menus::available_items_template, WP_Customize_Widgets::output_widget_control_templates, and WP_Customize_Nav_Menus::enqueue_scripts when the theme doesn't support menus nor widgets? It doesn't seem to me that there should ever be a call to get_panel( 'nav_menus' ) when we know that there won't be a panel with that ID.

westonruter commented on PR #3230:


9 months ago
#40

Yes. Really the Widgets and Nav Menus components should be disabled entirely. There is a filter for that: `customize_loaded_components`. However, I do not believe it will work here because it has to run when WP_Customize_Manager is constructed which happens at plugins_loaded, and thus before the theme is loaded. This means it would run too early to check if wp_is_block_theme(), right?

noisysocks commented on PR #3230:


9 months ago
#41

Yeah that's right. Bummer!

OK, marking as inactive seems reasonable all things considered 👍

#42 @desrosj
8 months ago

@westonruter Are you able to get this refined and committed prior to RC on this upcoming Tuesday?

#43 @westonruter
8 months ago

  • Keywords commit added

@desrosj yes, I can commit the PR today. My intention is to commit the PR as-is unless there is additional feedback.

#44 @westonruter
8 months ago

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

In 54419:

Customize: Prevent PHP notice in Customizer when using block theme.

Use the customize_panel_active filter to deactivate the Menus panel instead of overriding the check_capabilities method. This ensures the panel remains registered but is still hidden.

See #54888.
Fixes #54905.

Note: See TracTickets for help on using tickets.