WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#29583 closed defect (bug) (fixed)

The getter in WP_Error may cause an "Undefined property" notice

Reported by: bjornjohansen Owned by: SergeyBiryukov
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.0
Component: General Keywords: has-patch
Focuses: Cc:

Description

Since the getter doesn't check whether the requested property actually exists before trying to return it, it may cause an "Undefined property" notice.

This happens on the update page in wp-admin if the compatibility of a plugin is unknown.

Attached patch sets default return value to null and only return the value of the requested property if it actually is set.

Attachments (3)

class-wp-error.php.patch (343 bytes) - added by bjornjohansen 5 years ago.
29583.patch (2.3 KB) - added by SergeyBiryukov 5 years ago.
29583-class-wp-error.php-brianlayman.patch (425 bytes) - added by BrianLayman 5 years ago.
My version of the fix to get rid of the notices…

Download all attachments as: .zip

Change History (15)

#1 @bjornjohansen
5 years ago

  • Keywords has-patch added

#2 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 4.1

Reproduced the notice if plugins_api() returns a WP_Error:

PHP Notice:  Undefined property: WP_Error::$compatibility in wp-includes/class-wp-error.php on line 78

We should probably check if $info->compatibility is set in update-core.php, like we do for $info->tested. See 29583.patch.

#3 @jeremyfelt
5 years ago

+1 for 29583.patch. An alternative would be to ensure a filtered response of plugins_api() always includes compatibility and tested.

#4 @nacin
5 years ago

This (both existing and proposed code) seems like a lot of work to avoid an is_wp_error() check.

@BrianLayman
5 years ago

My version of the fix to get rid of the notices...

#5 @BrianLayman
5 years ago

I'm getting these notices too. It seems there are two issues here - one the overall handling of get calls on properties that don't exist and second the code that calls these things..

I'd propose that these are handled as two separate tickets. Clearing the notice does not need to be bogged down in the debate about how to handle the version check. I think serge's and Jeremy's code should be split off to another ticket.

NOTE: My original code returned a false. I've modified to make it match the null in bjorn's code. Null is probably correct, but TBH I'd have to test the exact code to be fully satisfied of which the notice producing code originally returned.

#6 follow-up: @bjornjohansen
5 years ago

The same notice will show up in other places as well, due to missing is_wp_error() checks. Today I got it while using get_terms with 'hide_empty' => false and all the terms in the taxonomy were empty (will create separate ticket for that).

I suggest we do as Brian is proposing: Fix the getter and move the missing is_wp_error() checks to separate tickets (even though the cases might never be discovered as soon as the getter is fixed).

#7 in reply to: ↑ 6 @SergeyBiryukov
5 years ago

Replying to bjornjohansen:

The same notice will show up in other places as well, due to missing is_wp_error() checks.

That's the point. A missing is_wp_error() check indicates a developer error, we should not hide these notices.

#8 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 29825:

Avoid a PHP notice in list_plugin_updates() if plugins_api() returned a WP_Error object.

fixes #29583.

#9 @dd32
5 years ago

In 31382:

Updates: Display plugin update rows even for plugins which are not hosted by WordPress.org or the HTTP request times out on.
See #29583.
Fixes #30767 for trunk.

#10 @dd32
5 years ago

A note on the above change - As of 4.2 & [31138] WP_Error no longer has magic methods, but keeping this code rather than reverting as a reminder to others that it can be a non-object.

#11 @dd32
5 years ago

In 31383:

Updates: Display plugin update rows even for plugins which are not hosted by WordPress.org or the HTTP request times out on.
See #29583.
Merges [31382] to the 4.1 branch.
Fixes #30767.

#12 @SergeyBiryukov
5 years ago

#29103 was marked as a duplicate.

Note: See TracTickets for help on using tickets.