Introduce Widget Posts functionality

[westonruter]

Depends on Core patch in https://github.com/xwp/wordpress-develop/pull/85 which is now committed!

• Make sure that WXR import/export work as expected.
• Diagnose object cache issue when toggling enabled state.
• Fix Core unit test error.
• Ensure that widget instance data is preserved and correct during wp widget-posts migrate.
• Store widget instance as pretty-printed JSON in post_content for sake of revision history and searching.
• Implement the wp widget-posts import command which takes data from JSON input as opposed to wp widget-posts migrate which takes it from options.

Nice to haves

• Add object-caching for widget ID => post ID lookups. (See Widget Posts: Add object-caching for widget ID => post ID lookups #19)
• Stop loading inactive widget instances into the Customizer, as this adds needless page size. (See Prevent inactive widgets from being registered up-front #17)
• Consider doing batch widget_instance post retrieval via a post__in. This should be done when the Customizer is going to get all registered widget settings, and it should be done for all widget IDs in wp_get_sidebar_widgets(). This is less of a concern when an external object cache is used, since in that case each widget will only get a DB call when first loading the page (e.g. SELECT * FROM wp_posts WHERE ID = 1999 LIMIT 1). (See Widget Posts: Do batch up-front widget_instance post retrieval  #20)

[kasparsd]

I wonder if this should run only if get_option( self::ENABLED_FLAG_OPTION_NAME ) / during init() in #L87?

[westonruter]

@kasparsd good question, but no, as this is just to capture the WP_Widget instances for use whether the functionality is enabled or not. For instance, when doing wp widget-posts migrate it needs to have access to the list of WP_Widget objects, even if wp widget-posts enable hasn't been called yet.

[kasparsd]

@westonruter Yeah, I didn't think of that. I was mostly worried about the PHP memory usage but now I realize that the reference object always required when the widget-posts is enabled.

[westonruter]

Since it is an object, too, it will be stored by reference anyway. So there will be almost zero space required to store this additional array, since all of the objects are strewn throughout $wp_registered_widgets and friends anyway.

[kasparsd]

I finally figured out why wp widget-posts migrate is importing incomplete widget instance data for certain widgets -- because of sanitize_instance(). This is mostly due to how certain widgets use $old_instance as the primary set of data and update only certain attributes from the $new_instance.

Considering that the instance data in the option_widget_* is already supposed to be safe, we can probably skip this additional sanitization until we can ensure that no data is lost? An alternative would be to check if the sanitized instance data is different from the original and notify the user during the import.

[westonruter]

@kasparsd Great point. So what is happening right now is that the widget's update callback is getting called twice when the widget is updated via the admin or Customizer. But otherwise, if this method is called programmatically by some other means, specifically import_widget_instances(), then it only is called once. But if we know that the instance data is already sanitized, then we should skip calling it again.

So I propose that we add a 3rd $options argument to this update_widget() and insert_widget() methods, like:

$options = wp_parse_args( $options, array(
    'needs_sanitization' => true,
) );

And in the places we call update_widget() or insert_widget() in our codebase, since in all of our uses it should all be pre-sanitized either by the update callback having been called or the data coming straight from the options via migration, we can then supply array( 'needs_sanitization' => false ) which would then prevent this sanitization logic from being called by wrapping it in if ( $options['needs_sanitization'] ).

[kasparsd]

@westonruter +1

[westonruter]

Here's what I've drafted:

diff --git a/php/class-widget-posts.php b/php/class-widget-posts.php
index 08e3ea9..f1037b7 100644
--- a/php/class-widget-posts.php
+++ b/php/class-widget-posts.php
@@ -247,7 +247,8 @@ class Widget_Posts {
            try {
                $post = null;
                if ( ! $options['dry-run'] ) {
-                   $post = $this->update_widget( $widget_id, $instance );
+                   // When importing we assume already sanitized so that the update() callback won't be called with an empty $old_instance.
+                   $post = $this->update_widget( $widget_id, $instance, array( 'needs_sanitization' => false ) );
                }
                do_action( 'widget_posts_import_success', compact( 'widget_id', 'post', 'instance', 'widget_number', 'id_base', 'update' ) );
            } catch ( Exception $exception ) {
@@ -420,7 +421,8 @@ class Widget_Posts {
            }
            $widget_id = "$id_base-$widget_number";

-           $this->update_widget( $widget_id, $instance );
+           // Note that sanitization isn't needed because the widget's update callback has already been called.
+           $this->update_widget( $widget_id, $instance, array( 'needs_sanitization' => false ) );
        }

        foreach ( $widget_settings->unset_widget_numbers as $widget_number ) {
@@ -637,16 +639,25 @@ class Widget_Posts {
     *
     * @param string $id_base
     * @param array $instance
+    * @param array [$options] {
+    *     @type bool $needs_sanitization
+    * }
     * @return \WP_Post
     *
     * @throws Exception
     */
-   function insert_widget( $id_base, $instance = array() ) {
+   function insert_widget( $id_base, $instance = array(), $options = array() ) {
        if ( ! array_key_exists( $id_base, $this->widget_objs ) ) {
            throw new Exception( "Unrecognized widget id_base: $id_base" );
        }
+       $options = wp_parse_args( $options, array(
+           'needs_sanitization' => true,
+       ) );
+
+       if ( $options['needs_sanitization'] ) {
+           $instance = $this->sanitize_instance( $id_base, $instance );
+       }

-       $instance = $this->sanitize_instance( $id_base, $instance );
        $widget_number = $this->plugin->widget_number_incrementing->incr_widget_number( $id_base );
        $post_arr = array(
            'post_name' => "$id_base-$widget_number",
@@ -672,10 +683,17 @@ class Widget_Posts {
     *
     * @param string $widget_id
     * @param array $instance
+    * @param array [$options] {
+    *     @type bool $needs_sanitization
+    * }
     * @throws Exception
     * @return \WP_Post
     */
-   function update_widget( $widget_id, $instance = array() ) {
+   function update_widget( $widget_id, $instance = array(), $options = array() ) {
+       $options = wp_parse_args( $options, array(
+           'needs_sanitization' => true,
+       ) );
+
        $parsed_widget_id = $this->plugin->parse_widget_id( $widget_id );
        if ( empty( $parsed_widget_id ) ) {
            throw new Exception( "Invalid widget_id: $widget_id" );
@@ -685,17 +703,19 @@ class Widget_Posts {
        }

        $post_id = null;
-       $old_instance = array();
        $post = $this->get_widget_post( $widget_id );
-       if ( $post ) {
-           $post_id = $post->ID;
-           try {
-               $old_instance = $this->get_widget_instance_data( $post );
-           } catch ( Exception $e ) {
-               $old_instance = array();
+       if ( $options['needs_sanitization'] ) {
+           $old_instance = array();
+           if ( $post ) {
+               $post_id = $post->ID;
+               try {
+                   $old_instance = $this->get_widget_instance_data( $post );
+               } catch ( Exception $e ) {
+                   $old_instance = array();
+               }
            }
+           $instance = $this->sanitize_instance( $parsed_widget_id['id_base'], $instance, $old_instance );
        }
-       $instance = $this->sanitize_instance( $parsed_widget_id['id_base'], $instance, $old_instance );

        // Make sure that we have the max stored
        $this->plugin->widget_number_incrementing->set_widget_number( $parsed_widget_id['id_base'], $parsed_widget_id['widget_number'] );

However, I see that it is introducing a unit test failure:

1) CustomizeWidgetsPlus\Test_Core_With_Widget_Posts::test_wp_widget_save_settings
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Unit Tested'
+''

So I need to see why that is.

[westonruter]

@kasparsd I pushed up the change, in spite of the unit test failure: 66d7013

If you could figure out why it is failing, that would be awesome.

[westonruter]

Also to ensure that it fixes the problem with migrating widget instances.

[kasparsd]

@westonruter Confirmed that migration works as expected and so does adding and updating widgets in the customizer.