-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Support storing blocks in sidebars #14251
Conversation
cc. @WordPress/gutenberg-core @draganescu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @noisysocks nice work here 👍
I am able to request the GET endpoint with success:
wp.apiFetch({
path: '/wp/v2/sidebars/sidebar-1',
method: 'GET',
}).then(console.log);
I am able to apply widget reorders and removals:
wp.apiFetch({
path: '/wp/v2/sidebars/sidebar-1',
data: {
"blocks": [{
"name": "core/legacy-widget",
"attributes": {
"identifier": "search-2"
},
"innerBlocks": [],
"innerHTML": "",
"innerContent": []
}, {
"name": "core/legacy-widget",
"attributes": {
"identifier": "nav_menu-2"
},
"innerBlocks": [],
"innerHTML": "",
"innerContent": []
}]
},
method: 'POST',
}).then(console.log);
The changes get reflected on the widget screen 👍
When I try to add blocks using this code:
wp.apiFetch({
path: '/wp/v2/sidebars/sidebar-1',
data: {
"blocks": [{
"clientId": "0875de4f-2320-4b6b-bd69-40b048fa43ef",
"name": "core/paragraph",
"isValid": true,
"attributes": {
"content": "Hello",
"dropCap": false
},
innerHTML: "<p>Hello</p>",
innerContent: "<p>Hello</p>",
"innerBlocks": []
}, {
"clientId": "cedd2160-3cb8-4c39-b293-8453e7ec74ef",
"name": "core/paragraph",
"isValid": true,
"attributes": {
"content": "World",
"dropCap": false
},
innerHTML: "<p>World</p>",
innerContent: "<p>World</p>",
"innerBlocks": []
}, {
"name": "core/legacy-widget",
"attributes": {
"identifier": "calendar-4"
},
"innerBlocks": [],
"innerHTML": "",
"innerContent": []
}, {
"name": "core/legacy-widget",
"attributes": {
"identifier": "media_image-3"
},
"innerBlocks": [],
"innerHTML": "",
"innerContent": []
}, {
"name": "core/legacy-widget",
"attributes": {
"identifier": "calendar-3"
},
"innerBlocks": [],
"innerHTML": "",
"innerContent": []
}, {
"name": "core/legacy-widget",
"attributes": {
"identifier": "recent-posts-2"
},
"innerBlocks": [],
"innerHTML": "",
"innerContent": []
}, {
"name": "core/legacy-widget",
"attributes": {
"identifier": "search-2"
},
"innerBlocks": [],
"innerHTML": "",
"innerContent": []
}, {
"name": "core/legacy-widget",
"attributes": {
"identifier": "nav_menu-2"
},
"innerBlocks": [],
"innerHTML": "",
"innerContent": []
}]
},
method: 'POST',
}).then(console.log);
The blocks get ignored and just the legacy widgets get saved. Am I doing something wrong?
} else { | ||
$blocks[] = array( | ||
'name' => 'core/legacy-widget', | ||
'attributes' => array( 'identifier' => $item ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to add the other instance attributes of the widget here?
global $wp_registered_widgets; | ||
|
||
foreach ( $sidebars_widgets as $sidebar_id => $widgets ) { | ||
foreach ( $widgets as $index => $widget_id ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is throwing a PHP warning on my test environment when I go to the widgets page:
<b>Warning</b>: Invalid argument supplied for foreach() in <b>/Users/pc/dev/core/wordpres-develop/build/wp-content/plugins/gutenberg/lib/register.php</b> on line <b>262</b><br />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not getting that. Could you var_dump( $widgets )
for me and tell me what it is? I don't want to blindly put a if ( is_array( ... ) )
in here and miss a potential bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm getting error Warning: Invalid argument supplied for foreach() in /Users/pc/dev/core/wordpres-develop/build/wp-content/plugins/gutenberg/lib/register.php on line 62
.
The contents of var_dump are:
| array(17) {
-- | --
| [0]=>
| string(8) "search-2"
| [1]=>
| string(10) "nav_menu-2"
| [2]=>
| string(17) "my-marquee-widget"
| [3]=>
| string(18) "my-marquee-widget2"
| [4]=>
| string(10) "calendar-3"
| [5]=>
| string(10) "calendar-4"
| [6]=>
| string(13) "media_image-3"
| [7]=>
| string(14) "recent-posts-2"
| [8]=>
| string(10) "archives-2"
| [9]=>
| string(15) "media_gallery-2"
| [10]=>
| string(17) "recent-comments-2"
| [11]=>
| string(12) "categories-2"
| [12]=>
| string(13) "custom_html-2"
| [13]=>
| string(6) "text-2"
| [14]=>
| string(6) "meta-2"
| [15]=>
| string(10) "calendar-2"
| [16]=>
| string(13) "media_image-2"
| }
| array(1) {
| [0]=>
| string(45) "block-widget-12a002c7d5f52fd5b1030df9a1f5fcb4"
| }
| int(3)
Checking the sidebars_widgets options saved in the database it is possible to see that besides wp_inactive_widgets and sidebar-1 it contains [array_version] => 3
maybe this is the reason for this error.
} | ||
} | ||
|
||
if ( ! empty( $items ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the user the user removes all the widgets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Fixed in 678f65c.
} | ||
|
||
if ( 'core/legacy-widget' === $block['name'] ) { | ||
$items[] = $block['attributes']['identifier']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the user is using a widget for the first time? Should we create a new widget instance before referencing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this isn't quite correct. I think it speaks to my comment below:
In this PR I assumed that core/legacy-widget is responsible for updating the original widget. Your comment in https://github.com/WordPress/gutenberg/pull/13511/files#r261867151 has made me realise that this isn't correct, though.
I'll need to do some experimenting, but I am thinking that, when a legacy widget is updated, we store the updates in the core/legacy-widget block. This should be backwards compatible because rendering a core/legacy-widget delegates rendering to the_widget. It does mean that plugins that rely on directly reading a widget's options will stop working, though. We will also need to find a solution for updating callback widgets.
$items[] = array( | ||
'blockName' => $block['name'], | ||
'attrs' => $block['attributes'], | ||
'innerBlocks' => $block['innerBlocks'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
innerBlocks may also contain legacy widgets, right? For example, a user may add a columns block to put two widgets side by side. Should we recursively apply the legacy widget block logic to the innerBlocks?
As long as we have an identifier of the widget and its instance attributes I think the conversion is possible. Another question is if this conversion is automatic or not. E.g: If I have a search widget will I get automatically get a core/heading + core/search or will I get a core/legacy-widget that I can convert to core/search after? If after the transformation, I change core/search block how the operation is saved? I guess the widget is removed and we use the normal process to save blocks. If I change a legacy widget, do we remove the widget that existed and start using a normal block the "legacy-widget" as we do for migrated blocks? Or do we keep a reference in the block to the original widget and for this case we update the original widget? |
Hey @jorgefilipecosta, try using
I am leaning strongly towards the latter approach. That is, users that upgrade will see This matches what we do for posts with the You can imagine scenarios where e.g. a theme might render a text widget differently to a text block. I think that it's important that the user has a chance to confirm that changes made to the site's frontend look correct.
Yes, I think you have it. The changes will be saved into the
That's a really good question. In this PR I assumed that I'll need to do some experimenting, but I am thinking that, when a legacy widget is updated, we store the updates in the |
Hi @noisysocks,
It updates the widgets referenced in core/legacy-widget thougth.
In PR #14395 I'm proposing a solution to this type of widgets. |
@jorgefilipecosta: It's because Gotchas like this reinforce how cumbersome it is having to send |
@jorgefilipecosta @youknowriad: Any thoughts on the Outstanding questions I have in this PR's description? 🙂 |
bad4cc8
to
933a01a
Compare
Quick replies:
Can we make the endpoint "experimental" somehow or "internal", I suspect will need a few iterations so we can't get everything right on the first iteration. Landing this #14612 will help moving forward with this PR and trying it in context. |
Thanks Riad!
We don't have feature flagging in PHP but maybe we could use edit: Settled on |
18df0f6
to
4628a14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @noisysocks, I like the usage of content strings as the format for this endpoint it threats each sidebar as a post and makes the API's consistent.
What you think of simplifying the store mechanism and during the save merely save all the content as the instance attribute a "block widget" that is the only widget referenced in the sidebar? If the sidebar already references a block widget, we use it if not (the first time the user is changing the sidebar with Gutenberg) we create a new instance of the block widget with the sidebar content, or with just an id of a post where the post with the blocks is stored and we move all the widgets the sidebar had to inactive widgets.
cc: @aduth, @youknowriad as If I'm not in error one of you proposed something similar.
If we prefer to not go for this "simplification", we need to add a way to create instances for widgets so we can use widgets previously not used or deleted in the legacy widget block.
The updates seemed to work on my tests.
Now I managed to update the sidebar with:
wp.apiFetch({
path: '/__experimental/sidebars/sidebar-1',
data: { content: '<!-- wp:paragraph --><p>fghgfhfg</p><!-- /wp:paragraph --><!-- wp:paragraph --><p>fghgfhfg</p><!-- /wp:paragraph -->' },
method: 'POST',
})
I found a problem for nested blocks the inner content is not included in the content.
E.g.: if we execute the following command it is possible to see the content is not complete:
wp.apiFetch({
path: '/__experimental/sidebars/sidebar-1',
data: { content: '<!-- wp:columns --><div class="wp-block-columns has-2-columns"><!-- wp:column --><div class="wp-block-column"><!-- wp:paragraph --><p>a</p><!-- /wp:paragraph --></div><!-- /wp:column --><!-- wp:column --><div class="wp-block-column"><!-- wp:paragraph --><p>brdtertertreterewrew45</p><!-- /wp:paragraph --></div><!-- /wp:column --></div><!-- /wp:columns -->' },
method: 'POST',
}).then(console.log);
} | ||
|
||
public function get_item( $request ) { | ||
return rest_ensure_response( $this->get_sidebar_data( $request['id'] ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not in error the good practice to access attributes from a request is using the $request->get_param function e.g:$request->get_param( 'id' ).
die( 'Silence is golden.' ); | ||
} | ||
|
||
function gutenberg_output_block_widget( $options, $block ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this logic be on widgets.php file?
$blocks[] = array( | ||
'blockName' => 'core/legacy-widget', | ||
'attrs' => array( | ||
'identifier' => $item, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the widget has a class legacy widgets reference the widget by the class name and not by the id. We may add a special condition on legacy widgets block and reference existing class id widgets by their id if you think it is worth it, it would allow us to update existing widgets via legacy widget block and still save the widget in the same entry as before.
We also need some flags e.g: to specify if a widget is a callback widget or not.
} | ||
} | ||
|
||
gutenberg_set_sidebars_items( array_merge( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are saving the blocks as PHP serialization of the parsed blocks result. Maybe we should save the blocks as an HTML string using our serialization format as we do in all other places.
} | ||
|
||
if ( | ||
'core/legacy-widget' === $block['blockName'] && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the user used the legacy widgets block, to use a widget that was never used before, and does not has any instance yet?
global $wp_registered_widgets; | ||
|
||
foreach ( $sidebars_widgets as $sidebar_id => $widgets ) { | ||
foreach ( $widgets as $index => $widget_id ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm getting error Warning: Invalid argument supplied for foreach() in /Users/pc/dev/core/wordpres-develop/build/wp-content/plugins/gutenberg/lib/register.php on line 62
.
The contents of var_dump are:
| array(17) {
-- | --
| [0]=>
| string(8) "search-2"
| [1]=>
| string(10) "nav_menu-2"
| [2]=>
| string(17) "my-marquee-widget"
| [3]=>
| string(18) "my-marquee-widget2"
| [4]=>
| string(10) "calendar-3"
| [5]=>
| string(10) "calendar-4"
| [6]=>
| string(13) "media_image-3"
| [7]=>
| string(14) "recent-posts-2"
| [8]=>
| string(10) "archives-2"
| [9]=>
| string(15) "media_gallery-2"
| [10]=>
| string(17) "recent-comments-2"
| [11]=>
| string(12) "categories-2"
| [12]=>
| string(13) "custom_html-2"
| [13]=>
| string(6) "text-2"
| [14]=>
| string(6) "meta-2"
| [15]=>
| string(10) "calendar-2"
| [16]=>
| string(13) "media_image-2"
| }
| array(1) {
| [0]=>
| string(45) "block-widget-12a002c7d5f52fd5b1030df9a1f5fcb4"
| }
| int(3)
Checking the sidebars_widgets options saved in the database it is possible to see that besides wp_inactive_widgets and sidebar-1 it contains [array_version] => 3
maybe this is the reason for this error.
Reporting from #4770 (comment) as it is related. |
Thanks all (especially @jorgefilipecosta) for the feedback here. I'm hitting the pause button on this exploration work for a little bit while I focus on writing an RFC which describes the approach I'd like us to take. Stay tuned! |
If it is at all helpful, I have been thinking over this matter a lot. I believe we should do the following. On some future WordPress upgrade, say 5.4, run a migration script. This will take all the existing widgets stored in options and migrate them to either a reusable block or new post type "wp_widget". We can map attributes on core widgets easily to blocks. We can even take a copy of the existing data and have it as a piece of post meta, so no data is lost. We can even add a filter or action, so plugin / theme developers can do something custom on this migration if they so wish. All widget area assignment can be handled by a new taxonomy, and new terms for created on this migration and all the new widget posts can be assigned to these terms. This means api calls can be filtered by taxonomy to get each widget area (sidebar). This plan would need this migration script to run on upgrade and a feature flag returned, to use the old widget edit screen until this migration is complete. IMO, I believe gutenberg should keep block in post content for now. It means that we can use existing post / term apis for widget / sidebar implementation. It load the effort onto the backend. It means no new widgets api is required, as everything will be a post. This is an idea that is been floating around for a long time. See 35669 Let me know when you want feedback @noisysocks |
- Allows blocks to be stored in a WordPress sidebar along with widgets. - Adds a REST endpoint for viewing sidebars and their blocks. Any widgets in the sidebar are shown as core/core-legacy blocks. - Adds a REST endpoint for updating a sidebar's blocks. Any core/legacy-widget blocks are stored in a backwards compatible way. - Block data is serialized directly as an array into the 'sidebars_widgets' site option. - We intercept calls to wp_get_sidebars_widgets() and replace any blocks with a legacy callback widget. This allows blocks to be rendered via dynamic_sidebar(), and ensures that existing code that uses wp_get_sidebars_widgets() does not break.
Update the legacy widget when an `instance` array is received, and send down the `instance` array when possible.
4628a14
to
71544f2
Compare
Closes #14182 — see this issue for a lot of useful context and terminology.
Adds the ability to store blocks in WordPress sidebars alongside widgets.
A new
GET /__experimental/sidebars/<id>
endpoint allows you to view a sidebar. Any blocks in the sidebar are shown as is. Any widgets in the sidebar are shown ascore/legacy-block
blocks which are to be added in #13511.A new
PUT /__experimental/sidebars/<id>
endpoint allows you to update the blocks in a sidebar.Blocks are stored directly in the existing
sidebars_widgets
site option as a serialised PHP array. We interceptwp_get_sidebars_widgets()
andwp_set_sidebars_widgets()
and swap out blocks for ablock-widget
widget which is lazily registered usingwp_register_sidebar_widget()
.Callers that wish to see the block data can use the new
gutenberg_get_sidebars_items()
andgutenberg_set_sidebars_items()
methods.This approach has some big benefits:
dynamic_sidebar()
will continue to work and will support blocks in sidebars appear without requiring any changes.wp_get_sidebars_widgets()
orwp_set_sidebars_widgets()
will continue to work without requiring any changes./wp-admin/widgets.php
screen in WP Admin continues to work. Blocks can be re-ordered and removed, just not updated.When merged into WordPress Core, this approach will be a little simpler to implement as we can modify
wp_get_sidebars_widgets()
andwp_set_sidebars_widgets()
directly instead of abusing filters.Outstanding questions
How should blocks look when sent down via the API? Should we send down objects containingGoing to go with HTML, at least to start. See Support storing blocks in sidebars #14251 (comment).name
,attributes
andinnerBlocks
? Or should we send down HTML containing block delimiters so that the/sidebar
endpoint matches the/posts
and/blocks
endpoints?Does fetching and updating one sidebar at a time withLet's mark the API as experimental and iterate on this. See Support storing blocks in sidebars #14251 (comment).GET /sidebars/<id>
andPUT /sidebars/<id>
make sense? Will the new widgets screen display an editor for all sidebars at the same time? Should we instead support viewing and updating multiple sidebars at once?How will conversion fromConversion will happen on the frontend. See Support storing blocks in sidebars #14251 (comment).core/legacy-widget
to e.g.core/search
work on the frontend? Are we sending down enough data for this to happen? This depends a lot what happens with Try Legacy widget block #13511.Outstanding tasks
instance
attribute on acore/legacy-widget
blockget_schema()