Make WordPress Core

Opened 9 years ago

Last modified 3 weeks ago

#37026 accepted defect (bug)

PHP Notice: Trying to get property of non-object in wp-admin\nav-menus.php on line 836

Reported by: skylarkcob's profile skylarkcob Owned by: sabernhardt's profile sabernhardt
Milestone: 7.0 Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-patch needs-copy-review changes-requested
Focuses: Cc:

Description

I'm using WordPress 4.5.2 and get this PHP Notice:

PHP Notice:  Trying to get property of non-object in wp-admin\nav-menus.php on line 836

When I export and import database for using on localhost (one database content for two sites), two sites using different theme and have different menu location.

My first site has menu location Mobile, when I use this database on new site with Twenty Sixteen theme, it doesn't have this location, so the notice message appeared.

The menu location on Menu Settings section looks like this:

Mobile (Currently set to: )

Plese add a conditional function to check if is a nav menu object before get its name.

wp_get_nav_menu_object( $menu_locations[ $location ] )->name

It's not an error, but I don't want to see this notice message when coding theme. Thank you.

Attachments (3)

37026.patch (1.5 KB) - added by Frozzare 9 years ago.
37026.2.patch (1.0 KB) - added by sabernhardt 6 months ago.
uses is_nav_menu() to check for false, an error or if the taxonomy is not 'nav_menu'
37026.3.patch (1.0 KB) - added by apermo 6 months ago.
Improved order in if from cheapest to most expensive for good measure.

Download all attachments as: .zip

Change History (13)

@Frozzare
9 years ago

#1 @Frozzare
9 years ago

  • Keywords has-patch added

Added a condition checking if the menu can be used before it's used. This condition exists in other places in the core when wp_get_nav_menu_object is used.

@sabernhardt
6 months ago

uses is_nav_menu() to check for false, an error or if the taxonomy is not 'nav_menu'

#2 @sabernhardt
6 months ago

  • Milestone set to Future Release

Related: #63484

At first, checking for false seemed to be enough because the docblock for wp_get_nav_menu_object() said it returns either WP_Term or false.
<?php if ( ! empty( $menu_locations[ $location ] ) && $menu_locations[ $location ] !== $nav_menu_selected_id && wp_get_nav_menu_object( $menu_locations[ $location ] ) ) : ?>

However, is_nav_menu() also checks for an error or incorrect taxonomy, so I used that instead in the refreshed patch.
<?php if ( ! empty( $menu_locations[ $location ] ) && is_nav_menu( $menu_locations[ $location ] ) && $menu_locations[ $location ] !== $nav_menu_selected_id ) : ?>

#3 @apermo
6 months ago

#63484 was marked as a duplicate.

#4 @apermo
6 months ago

I've reviewed @sabernhardt s Patch and it will fix #63484 and this since the additional call of is_nav_menu( $menu_locations[ $location ] ) will prevent wp_get_nav_menu_object() from returning false.

One minor improvement on it, I've changed the order, so that is_nav_menu( $menu_locations[ $location ] ) as the most expensive function will be called last, it likely won't make any measurable difference since it's backend code that's not regularly called, but it's good practice to sort conditions from cheap to expensive. And there are performance tickets just doing this.

@apermo
6 months ago

Improved order in if from cheapest to most expensive for good measure.

#5 @apermo
6 months ago

  • Version changed from 4.5.2 to 3.0

#6 @sabernhardt
6 months ago

  • Milestone changed from Future Release to 6.9

#7 @SirLouen
6 months ago

  • Keywords needs-copy-review changes-requested added

Combined Bug Reproduction and Patch Testing Report

Description

🟠 This report validates that the indicated patch works but with some caveats

Patch tested: https://core.trac.wordpress.org/attachment/ticket/37026/37026.3.patch

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 137.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-One 2.5
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Testing Instructions

I cannot really reproduce this through regular, only reproducible under certain forced conditions where regular menus are not being used (as the scenario commented by the OP). The only solution comes to adding a code to a plugin, to forcefully set an unexistent menu to an existing location.

  1. Add the following code to a plugin:
    $locations = get_nav_menu_locations();
    $locations['primary'] = 12345; 
    set_theme_mod('nav_menu_locations', $locations);
    
  2. 🐞 Error condition reproduced

Actual Results

  1. 🟠 Issue resolved with patch with concerns

Additional Notes

  • I have one main concern. With the patch, the fact is that the location is still assigned to an inexistent menu and nothing is informing of such. I feel that this patch is just hiding the error, but not dealing with it. In fact, the error is posing something that is intrinsically wrong. You can deal with the error by simply going into Menu > Manage Locations > And assigning the right Menu to the conflicting location.
  • My suggestion would be to add a notice explaining what is going on with this error. Something like

"Your $menu_locations[ $location ] location has a wrong menu assigned. Please, assign a valid menu"

  • I'm assuming that my error is a little different because it has been 9 years after the report was done.

Supplemental Artifacts

Error condition

https://i.imgur.com/WGc42Q3.png

Last edited 3 weeks ago by SirLouen (previous) (diff)

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


3 weeks ago

#9 @welcher
3 weeks ago

  • Milestone changed from 6.9 to 7.0

This was reviewed in the 6.9 Bug Scrub today. The patch need a refresh and we're 1 day away from Beta 2 so I will move it the 7.0 milestone.

#10 @sabernhardt
3 weeks ago

  • Owner set to sabernhardt
  • Status changed from new to accepted
Note: See TracTickets for help on using tickets.