WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 7 weeks ago

#52728 reviewing defect (bug)

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

Reported by: dlh Owned by: hellofromTonya
Milestone: 5.9 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 (2)

52728.diff (2.1 KB) - added by dlh 5 months ago.
52728.2.diff (1.5 KB) - added by dlh 2 months ago.

Download all attachments as: .zip

Change History (11)

@dlh
5 months ago

#1 @SergeyBiryukov
5 months ago

  • Milestone changed from Awaiting Review to 5.8

#2 @jrf
5 months 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 5 months ago by jrf (previous) (diff)

#3 @hellofromTonya
3 months 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
3 months 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.

#5 @SergeyBiryukov
3 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@dlh
2 months ago

#6 @dlh
2 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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

#7 @dlh
8 weeks ago

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.

#8 @jrf
7 weeks ago

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.

#9 @hellofromTonya
7 weeks ago

  • Keywords needs-unit-tests added; has-unit-tests removed
  • Milestone changed from 5.8 to 5.9
  • Owner changed from SergeyBiryukov to 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.

Note: See TracTickets for help on using tickets.