[wp-trac] [WordPress Trac] #52728: Widgets: Uncaught TypeError in PHP 8 when using objects for settings

[WordPress Trac]

#52728: Widgets: Uncaught TypeError in PHP 8 when using objects for settings
---------------------------------------------+---------------------
 Reporter:  dlh                              |       Owner:  (none)
     Type:  defect (bug)                     |      Status:  new
 Priority:  normal                           |   Milestone:  5.8
Component:  Widgets                          |     Version:  4.3
 Severity:  normal                           |  Resolution:
 Keywords:  has-patch php8 needs-unit-tests  |     Focuses:
---------------------------------------------+---------------------

Comment (by 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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/52728#comment:4>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform