Facilitate widgets to be stored in posts instead of options

[westonruter]

Widget instance data has always been stored in wp_options. This worked fine when there were only a few widgets used on a site. When there are hundreds—or thousands—there are big performance problems. When initializing widgets, Core loads the entire settings arrays of all widgets even when they are not used (#23909). In the Customizer, changes to widget settings is very expensive due having to unserialize and serialize option arrays repeatedly (#32103). Additionally, widget settings options are by default autoloaded: this means that for widget-heavy sites using Memcached Object Cache will reach the 1MB limit and crash since the autoloaded options will no longer be cacheable (WordPress.com even ​block a site from loading in this scenario). Storage in autoloaded options also means widgets are susceptible to alloptions cache corruption (#31245). Bottom line: widgets stored in options are not scalable.

As I've mentioned elsewhere, widgets should ideally be stored in posts instead of options. In addition to improving the performance problems above, there are many advantaged to storing widgets as posts, including user attribution via post_author, revision history, import/export, querying, widget drafts, scheduled widgets, etc.

Changing the storage mechanism for widgets is a major change, but with a couple small changes to WP_Widget, Core can easily support alternative widget instance storage systems, all through the pre_option_widget_{$id_base} and pre_update_option_{$id_base} filters.

The WP_Widget change is to allow settings to be array-like objects, specifically ArrayIterator. A plugin then can implement the offsetGet() and current() methods for an ArrayIterator subclass to lazy-load data from posts only when it is needed. Otherwise, the only data needed for WordPress at widgets_init are shallow arrays of multi-widget numbers mapped to their corresponding widget instance post IDs. When accessing an item in the array, the offsetGet() method can then look-up the widget instance data and cache it for future lookups.

This backwards-compatible shim approach is similar to what @nacin did in #20103 for WP_Theme (see also ​How WordPress Evolves Without Breaking Everything).

Let's make the widget a first-class citizen in WordPress.

[westonruter]

​https://github.com/xwp/wordpress-develop/pull/85

[nacin]

I think we're likely going to see alloptions go away in #31245, something I'm willing to try in 4.3. I'd be curious to see arguments here that don't involve alloptions.

[westonruter]

Replying to nacin:

I think we're likely going to see alloptions go away in #31245, something I'm willing to try in 4.3. I'd be curious to see arguments here that don't involve alloptions.

As mentioned in the description, non-alloptions arguments for not using options for storing widget instances include:

• Loading all instance settings data for all widgets even when they are not ever used is wasteful and inefficient. Imagine if all posts in the database had to be loaded into memory each time WordPress renders a template.
• Updating a single widget instance is expensive when settings are stored in options because for each instance change, the entire dataset for the widget type has to be unserialized. (Especially in the Customizer, #32103.)
• For Memcached Object Cache specifically, when the number of widgets of a given type grows large enough it will breach the 1MB limit and no longer be cacheable.

Using a custom post type for storing widget instances seems like an elegant WordPress-way to store this data and it brings with it several benefits, as noted in the ticket's description.

I'm working on finishing a proof of concept plugin that makes use of widget instances being passed around as ArrayIterators instead of intrinsic arrays.

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/68979474d3264e96e67a79feb4e5488d531b3258

[westonruter]

OK, I have an implementation of post-based widget instance storage implemented in the ​Customize Widgets Plus plugin, via the ​Widget Posts pull request. This plugin depends on the Core patch attached to this ticket. And this is all I'm proposing for 4.3: the minor attached patch to ensure widget instance $settings can be ArrayIterator objects in addition to literal arrays, as the array_* functions in PHP don't work with objects.

[sboisvert]

This patch would help large sites such as WordPress.com be in a better position to handle large amounts of Widgets since all widgets are stored in one memcache key currently (so a hard limit of 1MB for the total of all options [including widgets] on a site)

[westonruter]

Remove change fixed in #32496

[ocean90]

Based on ​PHP's manual ArrayObject and ArrayIterator are part of SPL which can be disabled before 5.3. Looks like we need some class_exists() check here.

WP_Theme uses ArrayAccess which isn't part of SPL.

[westonruter]

Replying to ocean90:

Based on ​PHP's manual ArrayObject and ArrayIterator are part of SPL which can be disabled before 5.3. Looks like we need some class_exists() check here.

WP_Theme uses ArrayAccess which isn't part of SPL.

Good question, but this isn't actually required in PHP starting with 5.1, as I just checked: ​http://3v4l.org/SkMVd

The instanceof operator works on undefined classes in PHP≥5.1 as we can see there, so I don't think we need any class_exists() checks added.

[westonruter]

Remove IDE type hinting: ​https://github.com/xwp/wordpress-develop/commit/af7eca0dabf747c3e43b250f388d8a9ddd1c5694

[ocean90]

Replying to westonruter:

The instanceof operator works on undefined classes in PHP≥5.1 as we can see there, so I don't think we need any class_exists() checks added.

Good to know!

The change looks sane. We can call it a temp hook which gives us the opportunity to improve widgets outside of core. Hello feature plugins.

[westonruter]

In 32602:

Add support for WP_Widget::get_settings() returning ArrayIterator/ArrayObject instances.

Plugins can use pre_option_widget_{$id_base} filters to return ArrayIterator/ArrayObject instances instead of primitive arrays. This makes possible for widget instance data to be drawn from somewhere else than wp_options, such as a custom post type.

Add unit tests for widgets.

Fixes #32474.

[nacin]

Looks good! Thanks for those arguments. Those are great to see.

Is there a reason ArrayAccess shouldn't also be whitelisted here?

[westonruter]

Replying to nacin:

Looks good! Thanks for those arguments. Those are great to see.

Is there a reason ArrayAccess shouldn't also be whitelisted here?

Yes, because the getArrayCopy() method isn't defined as part of the ArrayAccess interface, while it is defined for those two classes that implement it. We need that method.

[Stagger Lee]

Good work.
I already see how you enriched plugins repository with some new 300 plugins, in some near and far away future.

Does it mean cache plugins would be able to refresh widgets data when post changed more easy ?

[westonruter]

Replying to Stagger Lee:

Does it mean cache plugins would be able to refresh widgets data when post changed more easy ?

Well, this provides another way to flush a widget cache via the save_post action. But you can still do this now in Core just inside of the widget's update() callback anyway. But here's how you can do it with a save_post action (untested):

<?php

/**
 * Caching depends on the Customize Widgets Plus plugin's Widget Posts module.
 * @Link https://github.com/xwp/wp-customize-widgets-plus/pull/12  
 */
class Foo_Widget extends WP_Widget {

        function __construct() {
                parent::__construct( 'foo', 'Foo' );
                if ( post_type_exists( 'widget_instance' ) ) {
                        add_action( 'save_post_widget_instance', array( $this, 'flush_output_cache' ) );
                }
        }

        function flush_output_cache( $post_id ) {
                $post = get_post( $post_id );
                if ( ! empty( $post ) ) {
                        // Note that a widget_instance's post_name is the widget_id.
                        wp_cache_delete( $post->post_name, 'widget_output' );
                }
        }

        function widget( $args, $instance ) {
                if ( post_type_exists( 'widget_instance' ) ) {
                        $cached = wp_cache_get( $this->id, 'widget_output' );
                        if ( false !== $cached ) {
                                echo $cached;
                                return;
                        }
                        ob_start();
                }

                // ...

                if ( post_type_exists( 'widget_instance' ) ) {
                        $content = ob_get_contents();
                        wp_cache_set( $this->id, $content, 'widget_output' );
                        ob_flush();
                }
        }
}

[Stagger Lee]

Replying to westonruter:

Thanks. I am asking more generally, not that I self can make cache plugin.

Latest information I had is (not sure was it WP Total cache, or WP Super cache plugin). I needed to open all plugin folders of plugins that gives custom widgets options. And put some commented HTML code inside so cache plugin can refresh that widget. (Or delete/purge whole cache)
Not acceptable at all, not at all this with touching core files of plugins. I am sorry if I have wrong information about it.

[Stagger Lee]

Hope you dont mind I go a bit offtopic. Want just to ask, can you do something like this for Users ?

Users are difficult to extend and add filters, actions. Post types have almost unlimited possibilities.

Last 2-3 months I had need for this. Struggled with avatars, and post types can be easier used as staff/team customization.
Did it with Pods, but missing synchonisation with Users.

Featured images as avatars, extending possibilities huge. Mean for other plugins to hook and do magic, etc..

Also on other trac you talk about simplifying backend and removing some options. User profile needs it most. All is one long list without groups, headers.

[HeadOnFire]

@Stagger Lee
offtopic
Users have usermeta just like posts plus you actually can register custom taxonomies for users and get almost the same unlimited possibilities as with post types. There's a WP_User object and WP_User_Query, custom capabilities etc. I do a lot of complex user-related stuff all the time. While there are some areas to improve, of course, currently we have pretty solid and flexible set of features.

[azaozz]

I'm not sure this should be in core. Don't see what is fixed by adding that conditional. If a plugin or a theme wants to use the (hacky) way of intercepting options get and set, why should this be in core and not in the plugin?

// When $settings is an array-like object, get an intrinsic array for use with array_keys(). 
if ( $settings instanceof ArrayObject || $settings instanceof ArrayIterator ) { 
    $settings = $settings->getArrayCopy(); 
}

return $settings;

Also if we ever change the way widgets data is stored (using CPTs sounds like a good idea), this will have no backwards compatibility. If we need to let plugins change the way widgets data is stored, a much better way would be to use actual dedicated filters, like we do in so many other places. Then we can maintain backwards compatibility.

[westonruter]

@azaozz: Thanks for raising these concerns. Let me try to address them and explain.

Don't see what is fixed by adding that conditional.

What is fixed is that plugins now have the option of filtering the widget_{$id_base} options to return ArrayIterator objects instead of intrinsic arrays, and WP_Widget will handle them successfully. The array_* functions that had been used in WP_Widget's methods (e.g. is_array(), array_key_exists(), array_keys()) were not all compatible with the new Array objects in PHP 5. Now with the Core patch, WP_Widget is more agnostic to what kind of iterable is being passed around.

If a plugin or a theme wants to use the (hacky) way of intercepting options get and set, why should this be in core and not in the plugin?

The patch is making it possible for plugins to do just this. The patch isn't intercepting the options get/set in Core. It's just gracefully handling the scenario where get_option() is filtered to return an ArrayIterator (or ArrayObject) as opposed to an intrinsic array. For an example of what a plugin can then do with this accommodation in Core, see the Customize Widgets Plus plugin's ​Widget Posts module (yes, shameless plug).

The bit of the patch you quote here is unfortunate because PHP (strangely) provides no getKeys() method for its Array objects. So that's why it is using the getArrayCopy() method to obtain an array of the keys. But note that this method bypasses the accessor methods like ArrayIterator::offsetGet() and ArrayIterator::current(), so if those methods are indeed lazy-loading widget instance data from posts, this will not be involved when getArrayCopy() is called.

When developing the Widget Customizer plugin, I had to directly manipulate widget options extensively in order for them to be previewed and saved as expected. Even now, the Customizer in Core is creating WP_Customize_Settings of type option to represent widget instances. Based on this, my assumption has thus been that advanced plugins working with widgets may also interact with the underlying widget instance data via get_option() and update_option(), meaning that the filters need to be applied at this level. WP-CLI, for instance, ​gets the underlying option data as opposed to accessing WP_Widget::get_settings(). I did a quick search on WordPress.org, and I see the Display Widgets plugin also interacts with widget options directly.

Also if we ever change the way widgets data is saved (using CPTs sounds like a good idea), this will have no backwards compatibility.

Nevertheless, the WP_Widget::get_settings() and WP_Widget::save_settings() methods already work with arrays, and these arrays are mappings of widget instance numbers to their corresponding widget instance arrays. Even if we do have Core switch to storing widgets in posts as opposed to options, we can't get around the fact that these methods have been implemented in this way. So we need to provide a way for these methods to efficiently pass around arrays of widget instances no matter the data backend. This is what ArrayIterator makes possible, and it is fully backwards compatible.

If we need to let plugins change the way widgets data is stored, a much better way would be to use actual filters, like we do in so many other places. Then we can maintain backwards compatibility.

So yes, we could introduce filters in WP_Widget::get_settings() and WP_Widget::save_settings() to be able to short-circuit the normal logic and return ArrayIterator objects instead of intrinsic arrays. But other methods in WP_Widget (_register, display_callback, and form_callback) also then would have to be patched, as in this ticket's patch, to be compatible. And as above, this wouldn't help plugins that actually operate at the lower option-level.

Instead, without introducing new filters, we can leverage the existing pre_option_widget_{$id_base} and pre_update_option_widget_{$id_base} ones.

The patch is all backwards-compatible because without any plugins filtering pre_option_widget_{$id_base}, it will continue to work with intrinsic arrays as it always has. But then if a plugin does return ArrayIterator objects here instead, it is still backwards-compatible because these objects have the same interface as intrinsic arrays, as they are both iterables.

Sorry for the long(-winded) reply.

[obenland]

@azaozz, @westonruter, can this be closed? Or does it need more discussion?

[azaozz]

Replying to westonruter:

So yes, we could introduce filters in WP_Widget::get_settings() and WP_Widget::save_settings() to be able to short-circuit the normal logic and return ArrayIterator objects instead of intrinsic arrays. But other methods in WP_Widget (_register, display_callback, and form_callback) also then would have to be patched, as in this ticket's patch, to be compatible. And as above, this wouldn't help plugins that actually operate at the lower option-level.

Instead, without introducing new filters, we can leverage the existing pre_option_widget_{$id_base} and pre_update_option_widget_{$id_base} ones.

I meant that if core stops using options for saving the widgets data, the existing pre_option_* filters will never run, so all plugins that use them will break. And there is no way for core to add backwards compatibility to prevent that breakage. Of course the authors of these plugins will probably update them if/when that happens, it's just that using low-level filters for commonly needed tasks is not a good idea :)

Also, if we had new filters, we could pass either simple arrays or ArrayIterator objects without any back-compat concerns (see below).

...plugins now have the option of filtering the widget_{$id_base} options to return ArrayIterator objects instead of intrinsic arrays, and WP_Widget will handle them successfully.
...
The patch is all backwards-compatible because without any plugins filtering pre_option_widget_{$id_base}, it will continue to work with intrinsic arrays as it always has. But then if a plugin does return ArrayIterator objects here instead, it is still backwards-compatible because these objects have the same interface as intrinsic arrays, as they are both iterables.

Right, the patch is backwards compatible with core running only one plugin. However if two or more plugins use these filters, one can pass ArrayIterator, and the other (older) may not handle that successfully.

What we are doing here is allowing the data type passed by a filter to be changed. So an existing plugin that uses the filter and expect simple array, may get ArrayIterator object instead.

I agree, the chance of this happening for these filters is really low. I suppose we can accept the fact that some plugins will have to be updated to avoid conflicts with other similar plugins. Lets close this for now and hope any eventual incompatibilities between plugins will get resolved quickly.

[westonruter]

In 33696:

Widgets: Switch back to using array_key_exists() instead of isset() for widget instance existence check.

Reverts unnecessary change in [32602] since array_key_exists() does actually work with ArrayIterator objects.

See #32474.
Fixes #33442.

[westonruter]

In 33721:

Widgets: Switch back to using array_key_exists() instead of isset() for widget instance existence check.

Reverts unnecessary change in [32602] since array_key_exists() does actually work with ArrayIterator objects.

Merges [33696] to the 4.3 branch.
See #32474.
Fixes #33442 for the 4.3 branch.

[westonruter]

In 35272:

Widgets: Modify unit test assertion to be compatible with widget_nav_menu option being filtered by plugin to return ArrayIterator.

Modifies assertion added in [35100].

See #26876.
See #32474.

[westonruter]

The follow-up ticket proposing to actually store widgets as a custom post type instead of options in Core: #35669