WordPress.org

Make WordPress Core

Opened 7 weeks ago

Last modified 79 minutes ago

#52728 new defect (bug)

Widgets: Uncaught TypeError in PHP 8 when using objects for settings

Reported by: dlh Owned by:
Milestone: 5.8 Priority: normal
Severity: normal Version: 4.3
Component: Widgets Keywords: has-patch php8 needs-unit-tests
Focuses: Cc:

Description

Support for returning ArrayIterator and ArrayObject objects from WP_Widget::get_settings() was added in [32602]. In PHP 8, a fatal error now occurs when one of these objects is passed to array_key_exists() in WP_Widget::display_callback().

The attached patch would add a private method to WP_Widget for getting the settings as an array for use with array_key_exists() and other array-only functions, moving some logic already in WP_Widget::_register().

Related: [33696] for #33442. At the time, array_key_exists() supported objects, and it was used deliberately in this location.

Attachments (1)

52728.diff (2.1 KB) - added by dlh 7 weeks ago.

Download all attachments as: .zip

Change History (5)

@dlh
7 weeks ago

#1 @SergeyBiryukov
7 weeks ago

  • Milestone changed from Awaiting Review to 5.8

#2 @jrf
7 weeks ago

  • Keywords needs-unit-tests added

Notwithstanding the note about this in the PHP manual:

For backward compatibility reasons, array_key_exists() will also return true if key is a property defined within an object given as array. This behaviour is deprecated as of PHP 7.4.0, and removed as of PHP 8.0.0.

To check whether a property exists in an object, property_exists() should be used.

Source: https://www.php.net/manual/en/function.array-key-exists.php

... I wonder if this isn't actually a bug/missing feature in PHP itself.

Both the ArrayIterator class as well as the ArrayObject class implement ArrayAccess and implement the offsetExists() method.

That method is documented to be invoked when isset() or empty() is used, but I wonder if it shouldn't also be invoked when array_key_exists() is called.

Other than that, the patch looks good, though I'd like to see some tests with widgets using ArrayObjects or ArrayIterators being added. These tests should fail on PHP 8.0 without this patch and would safeguard the PHP 8.0 compatibility. (and could possibly unearth any other hidden PHP 8.0 incompatibilities in the WP_Widget)

Last edited 7 weeks ago by jrf (previous) (diff)

#3 @hellofromTonya
2 hours ago

... I wonder if this isn't actually a bug/missing feature in PHP itself.

Looking bugs.php.net, there is already a bug report that array_key_exists does not support ArrayAccess: https://bugs.php.net/bug.php?id=80162.

Note:
ArrayIterator implements ArrayAccess https://www.php.net/manual/en/class.arrayiterator.php.

#4 @jrf
79 minutes ago

Just looked at the patch again and started wondering why the extra method is needed when changing the array_key_exists() to isset() would solve this just as easily and would make for a much simpler patch.

I've read the complete discussion in #33442 and my take away from that discussion is basically that there are plugins/themes out there doing it wrong.

At the time, the change from array_key_exists() to isset() got reverted back to array_key_exists() as "it doesn't have any downside".

This ticket now, correctly calls out that as of PHP 8.0, it does have a downside.

So, I've dived in to try and understand how this error can ever really happen in the first place.

$instances is set via a call to $this->get_settings() and that method already does a check for $settings being an array or instance of one of the supported classes.

This error then, can only ever occur if a plugin/theme would overload the Widget::get_settings() method and doesn't have the same safeguards in place, or, as was the case in #33442, people overload the Widget::update() method but do not return the $new_instance (or false).

In both those cases, IMO, this is very much a case of plugins doing it wrong and not something WordPress should "fix" in the WordPress Core code - other than possibly adding a _doing_it_wrong() notice, but to be fair, that would be overkill, as the Widget::get_settings() method basically already does the validation and if people overload methods, they should at least comply with the expected return type.

WordPress should not cater or facilitate unlimitedly to plugins doing it wrong. IMO with this now being a fatal error in PHP 8.0, there is now a good argument to change it back to isset().

Refs:

So, having said that, a much simpler patch would suffice, preferably accompanied by some tests, but other than that, I don't think we should bend over backwards for people doing it wrong.

Note: See TracTickets for help on using tickets.