classic widgets with no settings won't show up in 5.9

[joncampbell]

Before 5.9 the WP_Widget class would use array_key_exists (within the display_callback method) when loading the widget, but now it uses isset. ​https://github.com/WordPress/WordPress/commit/df4834caea2c4e2be8595c12bb98b2849cdcf0c2

We have some widgets with no settings so the instance is there, but has a value of null and because of that, isset returns false. This causes the widget to not load at all.

Would really like these setting-less widgets to work again, on PHP 7.4 and 8.

My attempt at a way to allow widgets that don't have any settings to show up.

The change in 5.9 to isset within the display_callback made it so my settings array that has a value of null for the instance number hides my widget. I would like it if we allowed for objects and used isset for that but if the $instances is an array we can continue using array_key_exists so that widgets with no settings can work.

I recently added a ticket about the issue at https://core.trac.wordpress.org/ticket/55121 and hoping this solves that.

Trac ticket:

[costdev]

Hi @joncampbell, welcome to Trac and thanks for the patch!

Unit tests are passing and additional testing for PHP 5.6-8.1 ​shows the expected outcomes.

Moving to 5.9.1 milestone as this is a potential regression introduced in 5.9.

[costdev]

Pinging @jrf for a quick check here.

I see in your comment on #52728 that you refer to situations where extenders may be doing it wrong. Can you confirm whether this may be one of those times?

[costdev]

After looking into this some more, I believe this was considered doing it wrong in previous discussions. See here and the related replies.

However, given that this has come up again, I think we should consider one of the following ways forward:

• If it definitely is doing it wrong and BC doesn't need to be taken into account, consider adding _doing_it_wrong to highlight this to extenders.
  • This lets site owners and extenders know upon updating what the issue is and how to resolve it.
  • _doing_it_wrong() is used in several areas of Core even though the documentation already states the expected type(s).
  • It may be worth making a retrospective update to the ​5.9 Misc dev note and/or the ​5.9 PHP 8.0/8.1 dev note.
• If it isn't doing it wrong, consider the PR on this ticket.
  • Side note: The PR doesn't cover null values on ArrayObject/ArrayIterator, this may need to be added.

[mukesh27]

@costdev Agree with your feedback.

We also needs to update unit test as per Changeset [52173]

[joncampbell]

@costdev
I totally get it now.
The person who developed the widget that stopped working in 5.9 had an update() method that was empty and returned nothing.

I removed that from the custom widget class and now when the widget saves it stores an empty array so that the widget loading on the front end has an empty array of settings instead of null.

Feel free to get rid of this fix if the only way people would have a null settings is if they did it wrong.

[jrf]

Thanks for the ping @costdev.

I stand by my original analysis in https://core.trac.wordpress.org/ticket/52728#comment:4. It is the plugin which is doing it wrong. The fix which was put in place for #52728 is correct and should stay as is.

Suggest: close.

[costdev]

@jrf What do you think about adding a _doing_it_wrong() like elsewhere in Core?

[jrf]

@costdev See my original comment in which I discuss that as well. Short answer: no

[costdev]

@jrf Thanks! I'll close this ticket as invalid.

Related tickets for others who come across this: #33442, #52728

[peterwilsoncc]

I'm reopening this and putting it back on the 5.9.1 milestone.

A ​search of the plugin directory shows there is a large number of sites potentially affected.

I did a search for the one liner function update($new_instance, $old_instance) {} -- unfortunately I can't use wpdirectory.net to search for implementations that include line breaks (PSR-2, etc).

Given WP can work around it, I'd like to do so. A @return void isn't optimal for the update function, but I am not sure it is doing it wrong (to use the common vernacular against my better judgement).

[jrf]

@peterwilsoncc While I appreciate what you are saying, it is not WordPress' job to be forgiving for every single type of developer error.

To quote my previous comment:

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

The search you did on WP Directory shows 20 plugins affected, 1 with 4000 installs, 1 with 1000, 8 with less than 500 installs and 10 with 0 installs.

So realistically there are less than 6000 sites affected and of those, most won't be running PHP > 7.4.

I've checked the first few plugins on wp.org and most have been abandoned effectively (no update for > 3 years), so adding a doing it wrong to the WP code still wouldn't have any effect.

If anything, feel free to report the bug in the support fora of the 10 plugins with installs, but this is not something which should be accounted for in WP.

[peterwilsoncc]

So realistically there are less than 6000 sites affected and of those, most won't be running PHP > 7.4.

This is incorrect. As I mentioned, it's not possible for searches on wpdirectory.net to include line breaks so it is likely there is a greater number of sites affected.

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

I fundamentally disagree with this comment:

• I disagree with both the assertion and the phrasing the plugins were doing it wrong. The use of array_key_exists() was specifically to cater for this circumstance. Suboptimal, maybe, but certainly not wrong (whatever that means).
• WordPress's often stated goal is to maintain backward compatibility. As PHP becomes stricter about type this isn't always possible but where possible WP should do so. The compatibility break with switch to isset() in this code is a developer convenience rather than a necessity.

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

I agree this is something WP probably doesn't need to account for.

[jrf]

@peterwilsoncc When a plugin overloads a WP native method and then changes the return type from the documented expected return type to something completely different from and incompatible with the return type of the original method, do you really expect WP to handle that ?

Cause that's what's happening here.

[andraganescu]

Hey folks adding an extra opinion. The way I read this problem is:

• plugins do something which is wrong, overloading a method and returning something unexpected to core
• nevertheless that used to work, which makes things more complicated
• because we are fixing a problem by creating another one
• and we can avoid this with some harmless extra code

I agree with @jrf in that "is not WordPress' job to be forgiving for every single type of developer error", but @peterwilsoncc also has a point that "WordPress's often stated goal is to maintain backward compatibility".

So, in my head, we should try fixing for both situations: make PHP 8 not break and bad plugins continue to function, because it is possible, unless this is impossible. The decision to not support back compat problems is valid only if:

• the back compat code is likely to create future problems
• or if it is impossible today to both support back compat and the present.

The present or the future are more important than the past only if the past prevents us from accessing the present or the future. Otherwise we own our past, right? :)

So, as far as I undestand, the change appearing in 5.9 is also a backward compatibility problem because it worked "doing it the wrong way" before 5.9. To be clear, I am not a fan of this kind of patching, it's obvious that we're trying to provide for a principial developer error, even the OP decided this ticket can be closed. But patching to include both PHP 8 safety and back compat is owning to the fact that WP was not as strict as it could have been years ago.

[sc0ttkclark]

Just ran into this with some old code for a project that was written long ago by other developers.

Part of the issue is related to the My_Widget class example at ​https://developer.wordpress.org/themes/functionality/widgets/#your-widget-class

Yeah, the code is noted for how to use the update function elsewhere but it's not immediately apparent because the example has no return:

    public function update( $new_instance, $old_instance ) {
        // processes widget options to be saved
    }

And then the summary text right below it says something maybe perhaps more troubling:

update: Save the widget options to the database. If your widget doesn’t have any options, you can skip this function (although it is still best practice to include it even if it’s empty).

It's that "although it is still best practice to include it even if it’s empty" text that is very misleading, as it says you should include it even if it is empty. It should be saying it should at least return the $new_instance argument.

If nothing else happens to resolve this problem reported in this ticket, I definitely think this should be addressed there in the theme handbook page: ​https://developer.wordpress.org/themes/functionality/widgets/#your-widget-class

I still agree that it would be useful to check for and catch null above line 477 in WP_Widget::update_callback() in file class-wp-widget.php:

// Catch cases where the instance was not returned by an empty update() method.
if ( null === $instance ) {
    $instance = [];
}

NULL will always result it bugged widgets in WP 5.9+, but an empty array is a better way to deal with those cases.

[peterwilsoncc]

Replying to jrf:

@peterwilsoncc When a plugin overloads a WP native method and then changes the return type from the documented expected return type to something completely different from and incompatible with the return type of the original method, do you really expect WP to handle that ?

It's complicated.

Sometimes I do think WordPress should handle it, sometimes I do not. In this case, I think WordPress should handle it for a number of reasons:

• it doesn't require a great deal of code to handle void, null, etc -- ie, the cases in which there are reports
• it's an unannounced backward compatibility break
• WordPress has quite deliberately handled this use case in the past as seen in the earlier commit
• The documented return type (array) does not match the description Settings to save or bool false to cancel saving. The source of the confusion comes from WP (as Scott identifies in a secondary source above).

To reiterate from my comment above, I only think WordPress should handle this for widgets overloading the WP_Widget::update() method. WordPress shouldn't handle this of widgets overloading the Widget::get_settings() method as it is well outside the documented advice for developing a classic widget.

[peterwilsoncc]

I discussed this with @hellofromTonya to get another perspective and to collect the contributing factors:

• underlying cause is a ​reported PHP bug
• Confusing documentation contributed to the prevalence of this
  • return type is documented as array but descriptive text is array|false
  • WordPress developer docs lead people to using empty update() functions per Scott's comments above

As a path forward, Tonya and I figured:

• change the return type to array|false|null to match the documentation within code/the developer docs
• work around the PHP bug and comment that the code can be removed once the bug is fixed upstream

[jrf]

Let me suggest a slightly different path (which I still don't agree with, but seems I'm fighting a losing battle against foolhardiness):

• Update any and all developer docs/handbooks propagating the incorrect implementation of WP_Widget to show better code samples and to get rid of the incorrect code pattern.
• Do NOT change the return type to include null. array|false is the documented expected type as per the above, so let's keep it that way.
• Add a _doing_it_wrong() notice for when nullis received, as plugins should still be fixed for this incorrect implementation.
• Work around a received null value.

[SergeyBiryukov]

Replying to jrf:

• Update any and all developer docs/handbooks propagating the incorrect implementation of WP_Widget to show better code samples and to get rid of the incorrect code pattern.
• Do NOT change the return type to include null. array|false is the documented expected type as per the above, so let's keep it that way.
• Add a _doing_it_wrong() notice for when nullis received, as plugins should still be fixed for this incorrect implementation.
• Work around a received null value.

I tend to agree with this approach.

[audrasjb]

Moving to milestone 5.9.3 since we're about to release 5.9.2.

[audrasjb]

Moving to milestone 5.9.4.

[SergeyBiryukov]

Needs a new patch, per comment:20.

[audrasjb]

Moving this ticket to next major release since it wasn't addressed during this cycle. Anyone is welcome to move it back to 6.0.x minor releases cycle if a patch is ready to ship.

[karpstrucking]

workaround for null and doing_it_wrong

[karpstrucking]

patch uploaded. open to feedback on the wording of the _doing_it_wrong() output.

https://core.trac.wordpress.org/ticket/55121

[chaion07]

@joncampbell thank you for reporting this. We reviewed this ticket during a recent bug-scrub session. Considering the lack of progress during 6.1 cycle we are updating the milestone to Future Release. Should there be any significant progress before the release we can always roll back to the current one.

Cheers!

[hellofromTonya]

Reviewed this ticket to determine if it is an incompatibility with PHP 8.0 and should be listed in the known incompatibilities for WP 6.3.

Findings: no, this is not an incompatibility issue with PHP 8.0 and therefore, the php-compatibility focus should not be added to the ticket.