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

Render widget areas referencing blocks in a wp_area post on the website frontend #15651

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented May 15, 2019

Description

Here we implement a simple filter that makes a sidebar referencing a post_id reference a simple widget that renders the content of the post.

This is the first version and the most straightforward approach. To improve back-compatibility, I think we can parse the post. And replace legacy widget blocks to point to the original widget they referred, this change will allow users that go the previous screen to see the widgets they had there as they would typically see.

How has this been tested?

I executed this code in the browser console:

wp.apiFetch({
    path: '/__experimental/widget-areas/sidebar-1',
    data: { content: '<!-- wp:columns --><div class="wp-block-columns has-2-columns"><!-- wp:column --><div class="wp-block-column"><!-- wp:paragraph --><p>Col 1</p><!-- /wp:paragraph --></div><!-- /wp:column --><!-- wp:column --><div class="wp-block-column"><!-- wp:paragraph --><p>Col 2</p><!-- /wp:paragraph --></div><!-- /wp:column --></div><!-- /wp:columns -->' },
    method: 'POST',
}).then(console.log);

I went to the website frontend and verified the blocks appeared there.

@jorgefilipecosta jorgefilipecosta added [Type] Task Issues or PRs that have been broken down into an individual action to take [Feature] Widgets Screen The block-based screen that replaced widgets.php. labels May 15, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the add/logic-to-render-widget-areas-referencinga-wp-area-on-the-website-front-end branch 5 times, most recently from 8db7744 to ee18b08 Compare May 22, 2019 23:24
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

Nice work! Most of this looks familiar from my try/ branch, so of course I'm happy 🙂

When I test I see a PHP warning:

Screen Shot 2019-05-23 at 15 44 51

It looks like wp_widget_control() in wp-admin/includes/widget.php accesses global $sidebars_widgets; directly instead of calling wp_get_sidebars_widgets().

Can we work around this? Ideally global $sidebars_widgets should not contain the new data format so that it remains backwards compatible. Perhaps we could do a $sidebars_widgets = gutenberg_swap_out_sidebars_blocks_for_block_widgets( $sidebars_widgets ); somewhere—possibly in the init action.

lib/class-experimental-wp-widget-blocks-manager.php Outdated Show resolved Hide resolved
lib/widgets.php Outdated
* @param array $blocks Array of blocks.
*/
function gutenberg_blocks_to_widget( $blocks ) {
$widget_id = 'blocks-widget-' . md5( Experimental_WP_Widget_Blocks_Manager::serialize_block( $blocks ) );
Copy link
Member

Choose a reason for hiding this comment

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

Let's pull this function out of Experimental_WP_Widget_Blocks_Manager. We already have serialize_blocks so it makes sense in my mind that we'd have serialize_block as well.

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, in other PR @youknowriad recommended that we keep serialize_blocks inside Experimental_WP_Widget_Blocks_Manager to reinforce the experimental nature of the function. Given that recomendation, I guess we should also keep function gutenberg_blocks_to_widget( $blocks ) inside.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also what does serializing means? Is it's the same as the frontend? If it is, it means the blocks should be fully parsed and not just half-parsed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the serialization we have here is very simple and may not even work on all cases yet. It only needs to work for legacy widgets. I guess it is better to keep it inside the experimental class. And then we may have to rethink this to simply have functions like serialize_legacy_widgets which make their objectives more clear and may end up being public.

@@ -245,6 +257,9 @@ public static function serialize_blocks( $blocks ) {
* @return string String representing the block.
*/
public static function serialize_block( $block ) {
if( ! isset( $block[ 'blockName' ] ) ) {
return '';
Copy link
Member

Choose a reason for hiding this comment

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

Usually in PHP one returns false when there's an error. It should not impact serialize_blocks because PHP automatically converts false to '' when casting to a string.

php > var_dump( 'hello' . false . 'jorge' );
string(10) "hellojorge"

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented May 23, 2019

It looks like wp_widget_control() in wp-admin/includes/widget.php accesses global $sidebars_widgets; directly instead of calling wp_get_sidebars_widgets().

Can we work around this? Ideally global $sidebars_widgets should not contain the new data format so that it remains backwards compatible. Perhaps we could do a $sidebars_widgets = gutenberg_swap_out_sidebars_blocks_for_block_widgets( $sidebars_widgets ); somewhere—possibly in the init action.

I was not being able to reproduce the problem may be a plugin/theme is changing some behavior?
I tried to set the global in the init action but that's irrelevant as wp_get_sidebars_widgets changes the global variable anyway. I updated the filter to make sure the variable is also updated by the filter so if someone uses the global directly it should see the old structure. Let me know if this problem is persisting. I added some debug code and verified that in the line the error was appearing the global is set to the back compatible structure.

I also tried to use a lower level version where we filter directly using the filter option_sidebars_widgets so that we would change the result of get_option( 'sidebars_widgets' ) directly. That approach proved to be unfruitful given the mechanism of the options where we have cache the save become intermittent, for example saving in our endpoint sometimes caused the blocks to save as a widget block.

@jorgefilipecosta jorgefilipecosta force-pushed the add/logic-to-render-widget-areas-referencinga-wp-area-on-the-website-front-end branch 4 times, most recently from ff4fc68 to 597de7f Compare May 23, 2019 22:55
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

Yet again, I'm now not getting that error! Something must have been weird in my dev environment yesterday.

This looks good to get in and iterate on. There's a few minor suggestions that I'll fix up myself.

* @param array $options Widget options.
* @param array $arguments Arguments array.
*/
public static function gutenberg_output_blocks_widget( $options, $arguments ) {
Copy link
Member

Choose a reason for hiding this comment

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

Now that these methods are inside the experimental class, they don't need the gutenberg_ prefix.

Copy link
Member

@noisysocks noisysocks May 24, 2019

Choose a reason for hiding this comment

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

Renamed in 300e1c2.

* @param array $blocks Array of blocks.
*/
public static function gutenberg_blocks_to_widget( $blocks ) {
$widget_id = 'blocks-widget-' . md5( Experimental_WP_Widget_Blocks_Manager::serialize_block( $blocks ) );
Copy link
Member

Choose a reason for hiding this comment

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

Could be simply self::serialize_block.

Copy link
Member

Choose a reason for hiding this comment

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

Done in 2431bf6.

*/
public static function gutenberg_swap_out_sidebars_blocks_for_block_widgets( $sidebars_widgets_input ) {
global $sidebars_widgets;
if ( null === self::$unfiltered_sidebar_widgets ) {
Copy link
Member

Choose a reason for hiding this comment

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

If gutenberg_swap_out_sidebars_blocks_for_block_widgets is called again with a different $sidebars_widgets_input then this won't update its cached copy of self::$unfiltered_sidebar_widgets.

We probably want:

if ( $sidebars_widgets_input !== self::$unfiltered_sidebar_widgets ) {

Copy link
Member

Choose a reason for hiding this comment

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

Actually, not sure about this. Maybe it's not a problem. Is global $sidebars_widgets ever updated?

jorgefilipecosta and others added 3 commits May 24, 2019 08:57
Squashed commits:
[fa58e1f] Update lib/class-experimental-wp-widget-blocks-manager.php

Co-Authored-By: Robert Anderson <[email protected]>
[ee18b08] Render widget areas referencing blocks in a wp_area post on the website frontend.
@jorgefilipecosta jorgefilipecosta force-pushed the add/logic-to-render-widget-areas-referencinga-wp-area-on-the-website-front-end branch from 2431bf6 to c618839 Compare May 24, 2019 08:52
@jorgefilipecosta jorgefilipecosta merged commit 9b4f31a into master May 24, 2019
@jorgefilipecosta jorgefilipecosta deleted the add/logic-to-render-widget-areas-referencinga-wp-area-on-the-website-front-end branch May 24, 2019 09:05
@youknowriad youknowriad added this to the 5.8 (Gutenberg) milestone May 24, 2019
@youknowriad
Copy link
Contributor

Awesome work on all these widget screen related PRs @noisysocks @jorgefilipecosta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants