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

[dlh]

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.

[jrf]

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)

[hellofromTonya]

... 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.

[jrf]

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:

• https://core.trac.wordpress.org/ticket/33442#comment:3
• https://core.trac.wordpress.org/ticket/33442#comment:4

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.

[dlh]

@jrf Thanks for the thorough review! Your assessment of the situation makes sense to me, and 52728.2.diff​ contains the simpler diff.

As for unit tests, I'm having some trouble figuring out where to even start and stop. There isn't much coverage for any of the affected methods in WP_Widget on which to build.

For now, I've added a test to verify that WP_Widget::display_callback() doesn't generate a notice or error when the underlying setting is an ArrayObject, as it does today in PHP 7.4+, and I'm happy to expand on that if you think there are specific scenarios worth targeting.

[dlh]

If and when a change is committed for this ticket, I would offer that it be included in the miscellaneous dev note for the release (and would be happy to write the note), given that it will require plugins that have been able to "do it wrong" for a while to now update their code.

[jrf]

FYI: this ticket was discussed in ​today's pair programming session between @hellofromTonya and me with an impromptu guest appearance from @dlh.

The test in the current patch needs some additional work, which @dlh has graciously agreed to take on.

The short of it is, that for a test to cover this kind of PHP cross-version change, the test - without the patch applied - should pass on the older PHP versions and fail on PHP 8.0 due to the fatal error. In this particular case, the test will/should probably also throw a warning/fail on PHP 7.4 due to the PHP deprecation warning being thrown.

Once the validity of the test is confirmed, the source code patch is applied and the test should then pass on all supported PHP versions.

As things were, the test was failing on older PHP versions without the patch applied, so didn't sufficiently safeguard the particular PHP change which was intended to be safeguarded.

[hellofromTonya]

As the tests are not behaving as we'd expect, marking needs-unit-tests.

Today is 5.8 Beta 1. The tests and possibly the patch aren't ready for commit. Punting to 5.9.

@SergeyBiryukov reassigning myself as owner to keep track of it and help out for it to land in 5.9. Once ready, I'll ping you for the commit review.

[hellofromTonya]

Hello @dlh, is this on your radar to finish before 5.9 Beta 1 (Nov 16th)? Or should it move to 6.0?

[dlh]

Yep, this is on my radar. I can't guarantee I'll make it before Beta 1, but I'm going to try.

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

[dlh]

@hellofromTonya I've updated the last patch and submitted it as a Pull Request: ​https://github.com/WordPress/wordpress-develop/pull/1895

I've also checked locally that:

• The test passes with and without the patch in PHP 7.3.
• The test fails without the patch in PHP 7.4 because of the deprecation notice and passes with the patch.

The problem in my last patch was that I saved the ArrayObject instance directly to the options table. In PHP 7.3 and earlier, this object is serialized in such a way that core's is_serialized() doesn't detect it, so it's never unserialized on retrieval (see #17375). Because the object was never unserialized, it was never used for the widget's options during the test, and so the Test Title didn't render.

After re-reading #32474 and my own prior implementations of widget objects, I saw that the expected approach is to use the pre_option_* or option_* filters to supply the objects, so that's what happens now in the test.

[hellofromTonya]

In 52173:

Widgets: Use isset() in WP_Widget:: display_callback() to support ArrayIterator and ArrayObject.

[33696] introduced support returning ArrayIterator and ArrayObject objects from WP_Widget::get_settings().

Per the PHP manual, array_key_exists() stopped supporting this in PHP 8.0.0 and deprecated in PHP 7.4.0.

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.

This commit uses isset() instead of array_key_exists() which is supported on all current versions of PHP.

Includes unit tests.

Ref:

• ​https://www.php.net/manual/en/function.array-key-exists.php#refsect1-function.array-key-exists-notes

Follow-up to [32602], [33696].

Props dlh, hellofromTonya, jrf, sergeybiryukov.
Fixes #52728.

Committed via changeset https://core.trac.wordpress.org/changeset/52173.