Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try Legacy widget block #13511

Merged
merged 1 commit into from
Mar 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions gutenberg.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,21 @@ function the_gutenberg_project() {
<div id="metaboxes" class="hidden">
<?php the_block_editor_meta_boxes(); ?>
</div>
<?php
/**
* Start: Include for phase 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must find another way to extend this. It's slated for removal in #13569.

Copy link
Member

@aduth aduth Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need to occur where it is in the markup here? Or could it be done as hook handlers to admin_print_footer_scripts and admin_footer?

*/
/** This action is documented in wp-admin/admin-footer.php */
// phpcs:ignore WordPress.NamingConventions.ValidHookName.UseUnderscores
do_action( 'admin_print_footer_scripts-widgets.php' );

/** This action is documented in wp-admin/admin-footer.php */
// phpcs:ignore WordPress.NamingConventions.ValidHookName.UseUnderscores
do_action( 'admin_footer-widgets.php' );
/**
* End: Include for phase 2
*/
?>
</div>
<?php
}
Expand Down Expand Up @@ -223,6 +238,17 @@ function gutenberg_init( $return, $post ) {
*/
remove_action( 'admin_print_scripts', 'print_emoji_detection_script' );

/**
* Start: Include for phase 2
*/
// phpcs:ignore WordPress.NamingConventions.ValidHookName.UseUnderscores
do_action( 'admin_print_styles-widgets.php' );
// phpcs:ignore WordPress.NamingConventions.ValidHookName.UseUnderscores
do_action( 'admin_print_scripts-widgets.php' );
/**
* End: Include for phase 2
*/

/*
* Ensure meta box functions are available to third-party code;
* includes/meta-boxes is typically loaded from edit-form-advanced.php.
Expand Down
186 changes: 186 additions & 0 deletions lib/class-wp-rest-widget-updater-controller.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
<?php
/**
* Start: Include for phase 2
* Widget Updater REST API: WP_REST_Widget_Updater_Controller class
*
* @package gutenberg
* @since 5.2.0
*/

/**
* Controller which provides REST endpoint for updating a widget.
*
* @since 5.2.0
*
* @see WP_REST_Controller
*/
class WP_REST_Widget_Updater_Controller extends WP_REST_Controller {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're missing unit tests for this controller. WordPress has a really great API for writing REST API tests, check out trunk for examples: https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/rest-api?order=name


/**
* Constructs the controller.
*
* @access public
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we usually use @access.

*/
public function __construct() {
$this->namespace = 'wp/v2';
$this->rest_base = 'widgets';
}

/**
* Registers the necessary REST API route.
*
* @access public
*/
public function register_routes() {
register_rest_route(
$this->namespace,
// Regex representing a PHP class extracted from http://php.net/manual/en/language.oop5.basic.php.
'/' . $this->rest_base . '/(?P<identifier>[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*)/',
array(
'args' => array(
'identifier' => array(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

identifier feels oddly formal. We use id a lot in other places, including in wp-includes/widgets.php. Up to you!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @noisysocks,
I used identifier because this field is not the widget id, it may be the widget id for call back widgets after #14395 is landed, or it may be the class name for class widgets.
So in order to differentiate from the widget id I used identifier.

'description' => __( 'Class name of the widget.', 'gutenberg' ),
'type' => 'string',
),
),
array(
'methods' => WP_REST_Server::EDITABLE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this method make any changes on the server that are visible to future requests? If not, it's considered safe and is more of a GET request than a POST request.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, I used EDDITABLE because we may have a widget that has side effects (that's unexpected but we never know what plugins will do). With the improvements being made in #14395, for callback widgets now we apply a change so it makes sense to use editable.

'permission_callback' => array( $this, 'compute_new_widget_permissions_check' ),
'callback' => array( $this, 'compute_new_widget' ),
),
jorgefilipecosta marked this conversation as resolved.
Show resolved Hide resolved
)
);
}

/**
* Checks if the user has permissions to make the request.
*
* @since 5.2.0
* @access public
*
* @return true|WP_Error True if the request has read access, WP_Error object otherwise.
*/
public function compute_new_widget_permissions_check() {
// Verify if the current user has edit_theme_options capability.
// This capability is required to access the widgets screen.
if ( ! current_user_can( 'edit_theme_options' ) ) {
return new WP_Error(
'widgets_cannot_access',
__( 'Sorry, you are not allowed to access widgets on this site.', 'gutenberg' ),
array(
'status' => rest_authorization_required_code(),
)
);
}
return true;
}

/**
* Returns the new widget instance and the form that represents it.
*
* @since 5.2.0
* @access public
*
* @param WP_REST_Request $request Full details about the request.
* @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure.
*/
public function compute_new_widget( $request ) {
$url_params = $request->get_url_params();

$widget = $request->get_param( 'identifier' );
Copy link
Member

@noisysocks noisysocks Feb 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use the class name (e.g. 'WP_Widget_Search') to identify which widget to render, as one can register a widget in WordPress without using WP_Widget:

function my_widget_callback() {
	echo '<marquee>This is a legacy widget!</marquee>';
}
wp_register_sidebar_widget( 'my-widget', 'My cool widget', 'my_widget_callback' );

Widgets registered this way do not support user customisable settings have to manage their settings manually, and they cannot be used more than once per sidebar.

I haven't looked enough at your PR to make a recommendation on what to do, but I am thinking you might need to use the widget ID (e.g. 'search-2') instead of the class name (e.g. 'WP_Widget_Search') to reference which widget to render. The widget can then be looked up by its ID via the global $wp_registered_widgets array.

I'm currently dealing with the same roadblock while working on how to store blocks within our widgets architecture, so let me know if you want to chat about widgets—it's confusing! 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so I am thinking that we may have to make the block contain only the widget's identifier as an attribute, and nothing else:

<!-- wp:legacy-widget { "id": "search-2" } /-->

Rendering the widget's frontend would work by calling $wp_registered_widgets[ $id ]['callback']. Both legacy widgets and WP_Block widgets support this. Check out the source of dynamic_sidebar() in wp-includes/widgets.php for an example of this.

Rendering the widget's backend would work by calling $wp_registered_widget_controls[ $id ]['callback']. This will output a <form> which can be returned to the client. Both legacy widgets and WP_Block widgets support this. Check out the source of wp_widget_control() in wp-admin/includes/widgets.php for an example of this.

Updating the widget would work similar to how we currently support meta boxes: on save, we serialise the widget's <form> and send it off to the server. The server then calls $wp_registered_widget_controls[ $id ]['callback'] which will take care of inspecting $_POST and making updates as necessary. Both legacy widgets and WP_Block widgets support this. Check out the source of wp_ajax_save_widget() in wp-admin/includes/ajax-actions.php for an example of this.

Does that make sense, or have I missed something? I'm still learning how widgets are implemented in WordPress so take everything I say with a grain of salt! 🧂

Copy link
Member Author

@jorgefilipecosta jorgefilipecosta Mar 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use the class name (e.g. 'WP_Widget_Search') to identify which widget to render, as one can register a widget in WordPress without using WP_Widget:

function my_widget_callback() {
echo 'This is a legacy widget!';
}
wp_register_sidebar_widget( 'my-widget', 'My cool widget', 'my_widget_callback' );
Widgets registered this way do not support user customisable settings have to manage their settings manually, and they cannot be used more than once per sidebar.

Hi @noisysocks, good catch! I missed this type of widgets. I updated the code to correct the problem.

The sample code you provided now works as expected:

mar-03-2019 12-52-14

Copy link
Member Author

@jorgefilipecosta jorgefilipecosta Mar 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so I am thinking that we may have to make the block contain only the widget's identifier as an attribute, and nothing else:

<!-- wp:legacy-widget { "id": "search-2" } /-->

Hi @noisysocks
I thought about this option. It would probably have been easier to create an initial version following this path.

This path has some problems though:

  • The source of truth is not only the post content we are referencing something in the options database. It may not be clear that changing something in the options database (e.g: clean it) will change the way a normal post with a very simple legacy widget looks.

  • When we copy the code of the widget <!-- wp:legacy-widget { "id": "search-2" } /-->, and past it somewhere the expectation is that changing the pasted block will not change the original block. If that behavior is what the user desires, the user should use a reusable block.

  • The user may want to use a new widget right after installing it, for which no widget instance exists on the database. So we would need a mechanism to create new widget instances (a side effect of using the block).

  • If the user undoes a legacy widget usage immediately after a new instance was created, we need an undo side effect removing the widget from the database (undo side effects seems like something that may cause troubles). Redo side effect would also be needed to create a widget instance again.

  • If the user removes a legacy widget instance, we would have some troubles. We would need to decide if we remove it from the database or not. If we remove it from the database, we are in trouble if it was referenced meanwhile in other post. If we don't remove it from the database, each time legacy widgets are inserted the number of widget instances grows. Growing the number of widget instances, may not scale well given the way widgets are stored (serialized PHP objects).

Saving the widget instance as standard block attributes seemed to overcome this limitations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense Jorge. Thanks for thinking it through!

My only concern now is that it's not clear how we will support callback widgets that use both wp_register_sidebar_widget()and wp_register_widget_control(). We'll need to do this to remain backwards compatible with wp-admin/widgets.php.


global $wp_widget_factory;

if (
null === $widget ||
! isset( $wp_widget_factory->widgets[ $widget ] ) ||
! ( $wp_widget_factory->widgets[ $widget ] instanceof WP_Widget )
jorgefilipecosta marked this conversation as resolved.
Show resolved Hide resolved
) {
return new WP_Error(
'widget_invalid',
__( 'Invalid widget.', 'gutenberg' ),
array(
'status' => 404,
)
);
}

$widget_obj = $wp_widget_factory->widgets[ $widget ];

$instance = $request->get_param( 'instance' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused: why do we need both instance and instance_changes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The instance represents, the previous widget instance normally saved on the database (in our case save as the block attributes). Instance changes represent the fields that are being changed. That's how the widgets work, the update method receives the previous instance and the changes and returns a new instance.

if ( null === $instance ) {
$instance = array();
}
$id_to_use = $request->get_param( 'id_to_use' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the client need to provide this? What would be wrong with the server always using -1?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server uses this value to compute the id's used in the forms. Most JavaScript widgets have a check and if an id was initialized before they do nothing. So we need a unique id per widget instance if we used -1 all the time most JS widgets would only support a single instance.

if ( null === $id_to_use ) {
$id_to_use = -1;
}

$widget_obj->_set( $id_to_use );
ob_start();

$instance_changes = $request->get_param( 'instance_changes' );
if ( null !== $instance_changes ) {
$old_instance = $instance;
$instance = $widget_obj->update( $instance_changes, $old_instance );
jorgefilipecosta marked this conversation as resolved.
Show resolved Hide resolved
/**
* Filters a widget's settings before saving.
*
* Returning false will effectively short-circuit the widget's ability
* to update settings. The old setting will be returned.
*
* @since 5.2.0
*
* @param array $instance The current widget instance's settings.
* @param array $instance_changes Array of new widget settings.
* @param array $old_instance Array of old widget settings.
* @param WP_Widget $widget_ob The widget instance.
*/
$instance = apply_filters( 'widget_update_callback', $instance, $instance_changes, $old_instance, $widget_obj );
if ( false === $instance ) {
$instance = $old_instance;
}
}

$instance = apply_filters( 'widget_form_callback', $instance, $widget_obj );

$return = null;
if ( false !== $instance ) {
$return = $widget_obj->form( $instance );

/**
* Fires at the end of the widget control form.
*
* Use this hook to add extra fields to the widget form. The hook
* is only fired if the value passed to the 'widget_form_callback'
* hook is not false.
*
* Note: If the widget has no form, the text echoed from the default
* form method can be hidden using CSS.
*
* @since 5.2.0
*
* @param WP_Widget $widget_obj The widget instance (passed by reference).
* @param null $return Return null if new fields are added.
* @param array $instance An array of the widget's settings.
*/
do_action_ref_array( 'in_widget_form', array( &$widget_obj, &$return, $instance ) );
}

$id_base = $widget_obj->id_base;
$id = $widget_obj->id;
$form = ob_get_clean();

return rest_ensure_response(
array(
'instance' => $instance,
'form' => $form,
'id_base' => $id_base,
'id' => $id,
)
);
}
}
/**
* End: Include for phase 2
*/
85 changes: 67 additions & 18 deletions lib/client-assets.php
Original file line number Diff line number Diff line change
Expand Up @@ -872,34 +872,83 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
);
}

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As by the previous note, this function doesn't exist as of #13569.

All new functionality should extend the block editor as if it were any other plugin extending the core block editor.

* Start: Include for phase 2
*/

/**
* Todo: The hardcoded array should be replaced with a mechanisms that allows core blocks
* and third party blocks to specify they already have equivalent blocks, and maybe even allow them
* to have a migration function.
*/
$core_widgets = array( 'WP_Widget_Pages', 'WP_Widget_Calendar', 'WP_Widget_Archives', 'WP_Widget_Media_Audio', 'WP_Widget_Media_Image', 'WP_Widget_Media_Gallery', 'WP_Widget_Media_Video', 'WP_Widget_Meta', 'WP_Widget_Search', 'WP_Widget_Text', 'WP_Widget_Categories', 'WP_Widget_Recent_Posts', 'WP_Widget_Recent_Comments', 'WP_Widget_RSS', 'WP_Widget_Tag_Cloud', 'WP_Nav_Menu_Widget', 'WP_Widget_Custom_HTML' );

$has_permissions_to_manage_widgets = current_user_can( 'edit_theme_options' );
$available_legacy_widgets = array();
global $wp_widget_factory, $wp_registered_widgets;
foreach ( $wp_widget_factory->widgets as $class => $widget_obj ) {
if ( ! in_array( $class, $core_widgets ) ) {
$available_legacy_widgets[ $class ] = array(
'name' => html_entity_decode( $widget_obj->name ),
'description' => html_entity_decode( $widget_obj->widget_options['description'] ),
'isCallbackWidget' => false,
);
}
}
foreach ( $wp_registered_widgets as $widget_id => $widget_obj ) {
if (
is_array( $widget_obj['callback'] ) &&
isset( $widget_obj['callback'][0] ) &&
( $widget_obj['callback'][0] instanceof WP_Widget )
) {
continue;
}
$available_legacy_widgets[ $widget_id ] = array(
'name' => html_entity_decode( $widget_obj['name'] ),
'description' => null,
jorgefilipecosta marked this conversation as resolved.
Show resolved Hide resolved
'isCallbackWidget' => true,
);
}
/**
* End: Include for phase 2
*/

$editor_settings = array(
'alignWide' => $align_wide,
'availableTemplates' => $available_templates,
'allowedBlockTypes' => $allowed_block_types,
'disableCustomColors' => get_theme_support( 'disable-custom-colors' ),
'disableCustomFontSizes' => get_theme_support( 'disable-custom-font-sizes' ),
'disablePostFormats' => ! current_theme_supports( 'post-formats' ),
'titlePlaceholder' => apply_filters( 'enter_title_here', __( 'Add title', 'gutenberg' ), $post ),
'bodyPlaceholder' => apply_filters( 'write_your_story', __( 'Start writing or type / to choose a block', 'gutenberg' ), $post ),
'isRTL' => is_rtl(),
'autosaveInterval' => 10,
'maxUploadFileSize' => $max_upload_size,
'allowedMimeTypes' => get_allowed_mime_types(),
'styles' => $styles,
'imageSizes' => gutenberg_get_available_image_sizes(),
'richEditingEnabled' => user_can_richedit(),
'alignWide' => $align_wide,
'availableTemplates' => $available_templates,
/**
* Start: Include for phase 2
*/
'hasPermissionsToManageWidgets' => $has_permissions_to_manage_widgets,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem like this should be an editor setting. We have precedent for this sort of permissions check using REST API links.

'availableLegacyWidgets' => $available_legacy_widgets,
/**
* End: Include for phase 2
*/
'allowedBlockTypes' => $allowed_block_types,
'disableCustomColors' => get_theme_support( 'disable-custom-colors' ),
'disableCustomFontSizes' => get_theme_support( 'disable-custom-font-sizes' ),
'disablePostFormats' => ! current_theme_supports( 'post-formats' ),
'titlePlaceholder' => apply_filters( 'enter_title_here', __( 'Add title', 'gutenberg' ), $post ),
'bodyPlaceholder' => apply_filters( 'write_your_story', __( 'Start writing or type / to choose a block', 'gutenberg' ), $post ),
'isRTL' => is_rtl(),
'autosaveInterval' => 10,
'maxUploadFileSize' => $max_upload_size,
'allowedMimeTypes' => get_allowed_mime_types(),
'styles' => $styles,
'imageSizes' => gutenberg_get_available_image_sizes(),
'richEditingEnabled' => user_can_richedit(),

// Ideally, we'd remove this and rely on a REST API endpoint.
'postLock' => $lock_details,
'postLockUtils' => array(
'postLock' => $lock_details,
'postLockUtils' => array(
'nonce' => wp_create_nonce( 'lock-post_' . $post->ID ),
'unlockNonce' => wp_create_nonce( 'update-post_' . $post->ID ),
'ajaxUrl' => admin_url( 'admin-ajax.php' ),
),

// Whether or not to load the 'postcustom' meta box is stored as a user meta
// field so that we're not always loading its assets.
'enableCustomFields' => (bool) get_user_meta( get_current_user_id(), 'enable_custom_fields', true ),
'enableCustomFields' => (bool) get_user_meta( get_current_user_id(), 'enable_custom_fields', true ),
);

$post_autosave = gutenberg_get_autosave_newer_than_post_save( $post );
Expand Down
21 changes: 21 additions & 0 deletions lib/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,15 @@
// These files only need to be loaded if within a rest server instance
// which this class will exist if that is the case.
if ( class_exists( 'WP_REST_Controller' ) ) {
/**
* Start: Include for phase 2
*/
if ( ! class_exists( 'WP_REST_Widget_Updater_Controller' ) ) {
require dirname( __FILE__ ) . '/class-wp-rest-widget-updater-controller.php';
}
/**
* End: Include for phase 2
*/
require dirname( __FILE__ ) . '/rest-api.php';
}

Expand Down Expand Up @@ -43,6 +52,18 @@
if ( ! function_exists( 'render_block_core_latest_posts' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/latest-posts/index.php';
}


/**
* Start: Include for phase 2
*/
if ( ! function_exists( 'render_block_legacy_widget' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/legacy-widget/index.php';
}
/**
* End: Include for phase 2
*/

if ( ! function_exists( 'render_block_core_rss' ) ) {
require dirname( __FILE__ ) . '/../packages/block-library/src/rss/index.php';
}
Expand Down
19 changes: 19 additions & 0 deletions lib/rest-api.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,22 @@ function gutenberg_filter_oembed_result( $response, $handler, $request ) {
);
}
add_filter( 'rest_request_after_callbacks', 'gutenberg_filter_oembed_result', 10, 3 );



/**
* Start: Include for phase 2
*/
/**
* Registers the REST API routes needed by the legacy widget block.
*
* @since 5.0.0
*/
function gutenberg_register_rest_widget_updater_routes() {
$widgets_controller = new WP_REST_Widget_Updater_Controller();
$widgets_controller->register_routes();
}
add_action( 'rest_api_init', 'gutenberg_register_rest_widget_updater_routes' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have gutenberg_register_rest_routes() defined in rest-api.php. Should we un-deprecated this existing method and use it? cc. @aduth

/**
* End: Include for phase 2
*/
Loading