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

Experimental widgets editor - API and data model #24267

Closed
adamziel opened this issue Jul 29, 2020 · 8 comments
Closed

Experimental widgets editor - API and data model #24267

adamziel opened this issue Jul 29, 2020 · 8 comments
Assignees
Labels
[Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets

Comments

@adamziel
Copy link
Contributor

adamziel commented Jul 29, 2020

The current version of the experimental widgets screen is able to store blocks in widget areas using the following trick: In core, all the widgets are stored in the sidebars_widgets like this:

[
"sidebar-1" => ["custom-html-3", "categories-2"],
"sidebar-2" => ["recent-posts-2", "recent-comments-1"],
]

Let's say you add a paragraph block to the sidebar-1 using the new screen. The stored option now becomes:

[
"sidebar-1" => 12,
"sidebar-2" => ["recent-posts-2", "recent-comments-1"],
]

Where 12 is a post ID holding a content similar to:

<!-- wp:paragraph --><p>My fine paragraph</p><!-- /wp:paragraph -->
<!-- wp:legacy-widget {"widgetClass":"WP_Widget_Custom_HTML","id":"custom_html-2","idBase":"custom_html","number":3,"instance":{"title":"","content":""}} /-->
<!-- wp:legacy-widget {"widgetClass":"WP_Widget_Categories","id":"categories-4","idBase":"categories","number":2,"instance":{"title":"","count":0,"hierarchical":0,"dropdown":0}} /-->

And whenever a consumer of that option attempts to read it through e.g. get_option, there are filters in place to transform it back to the array format:

[
"sidebar-1" => ["blocks-widget-03192b1a8bae97c12a9f94129a8326ac"],
"sidebar-2" => ["recent-posts-2", "recent-comments-1"],
]

The result is that the existing/legacy widgets screen says there is only one widget called Block area:

Zrzut ekranu 2020-07-29 o 12 45 57

And making any changes on that screen (e.g. adding a calendar widget) breaks the stored data: the post ID is lost and the updated option looks like this:

[
"sidebar-1" => ["calendar-4", "blocks-widget-03192b1a8bae97c12a9f94129a8326ac"],
"sidebar-2" => ["recent-posts-2", "recent-comments-1"],
]

My big question is: Do we need to work around the widgets data model like that?

The idea of using a post great if it wasn't for the BC - it is possible to go from the post ID to the array of widgets, but this operation is not easily invertible (if at all). This means that once a user saves the widgets on the new screen, the legacy screen is no longer able to correctly handle any writes. Was that intended? If yes, then we could likely close this issue and proceed as is. If we want to retain BC, then let's consider an alternative way forward. CC @noisysocks @mtias @draganescu

An alternative could look like this:

[
"sidebar-1" => ["blocks-widget-2", "calendar-4", "blocks-widget-4"],
]

In this scenario, we would say that a "block area" is just a regular widget. There would be class called WP_Widget_Blocks (tbd) which would handle rendering (probably not editing) of the widget on the existing/legacy screen. The experimental widgets screen would be based on the data model above. Block content related to each "block area" could be stored as an option just like all the other widgets store their settings. It could also diverge from that standard a bit and use a post if it would play better with some larger vision of block areas. The main point here is not changing the format of the sidebars_widgets option and using a regular WP_Widget class to support.

@adamziel adamziel added [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Package] Edit Widgets /packages/edit-widgets labels Jul 29, 2020
@adamziel adamziel self-assigned this Jul 29, 2020
@adamziel
Copy link
Contributor Author

adamziel commented Jul 29, 2020

Case in point, there exists a feature plugin that adds a series of /widgets endpoints to the REST API: https://github.com/WP-API/widgets-endpoints

If we stick to the original data model and move forward with the WP_Widget_Blocks implementation, we could just reuse the code that already exists. Otherwise, the REST API bit will need a lot of thought to cover all the use cases.

@adamziel
Copy link
Contributor Author

adamziel commented Jul 29, 2020

Another good argument to preserve the existing data model - it is likely that there are multiple plugins out there that expect the array structure and would overwrite it on occasions.

@noisysocks
Copy link
Member

tl;dr: I think you're right about everything 😀


My big question is: Do we need to work around the widgets data model like that?

Not necessarily! @jorgefilipecosta and I landed on this design but there's nothing stopping us from tinkering with it while block widgets are still an experimental feature. We've collectively learned a lot in the year or so since this feature was first implemented.

This means that once a user saves the widgets on the new screen, the legacy screen is no longer able to correctly handle any writes. Was that intended?

The assumption was that this new screen would replace the old screen. This would mean that there is never a scenario where the legacy screen needs to perform writes to the new data structure because it is not possible to downgrade a site's version of WordPress.

This assumption is probably not correct, though. Note #24087 which proposes to let users opt in and out of the new screen by switching to and from a theme that supports block widgets.

I'm okay with axing this assumption. A design that lets users opt in and/or out of the new screen feels more "Gutenberg-ey". You'll recall that the block editor was introduced in a way that let users opt out by installing the Classic Editor plugin. The blocks would still be there, just as HTML comments. We can do something similar for widgets by using the existing HTML widget or a new blocks widget like you say.

In this scenario, we would say that a "block area" is just a regular widget. There would be class called WP_Widget_Blocks (tbd) which would handle rendering (probably not editing) of the widget on the existing/legacy screen. The experimental widgets screen would be based on the data model above. Block content related to each "block area" could be stored as an option just like all the other widgets store their settings.

I think this is the way to go! 🙂

It's a somewhat old idea. I recall @mtias suggesting it early on. You can see @aduth describing a similar approach in #14812 (comment).

From looking at that thread, I remember that my struggle with implementing this approach was that I could not figure out how to make it so that PUT /block-areas/:id would not unnecessarily delete and re-create (block) HTML widgets when updating the blocks in a block area.

What I failed to see is that we should perform the conversion from blocks to (block) HTML in the client and then store them using a more broadly useful /widgets API.

It could also diverge from that standard a bit and use a post if it would play better with some larger vision of block areas.

Alternatively we could look at (separately to this project!) making it so that all widgets are stored in a CPT. This has a lot of benefits and has been desired for a long time now. See https://core.trac.wordpress.org/ticket/35669.

If we stick to the original data model and move forward with the WP_Widget_Blocks implementation, we could just reuse the code that already exists. Otherwise, the REST API bit will need a lot of thought to cover all the use cases.

Agreed, and I think https://github.com/WP-API/widgets-endpoints is a much more useful API than /block-widgets as it covers use cases that exist right now such as using WordPress as the backing store for a headless website. See https://core.trac.wordpress.org/ticket/41683.

Another good argument to preserve the existing data model - it is likely that there are multiple plugins out there that expect the array structure and would overwrite it on occasions.

Probably this isn't that common—try searching https://www.wpdirectory.net to find out. But it's a bonus nonetheless!

@draganescu
Copy link
Contributor

Note #24087 which proposes to let users opt in and out of the new screen by switching to and from a theme that supports block widgets.

Wanted to note that the opting in to the screen itself is temporary. That optin will turn into opting in allowing blocks in sidebars, but the screen will be for everyone unless via plugin someone reverts it. But yes, there are ways to get the old screen back anyway.

@adamziel
Copy link
Contributor Author

Awesome, it seems like this issue is pretty much resolved. I started a draft PR here #24290

@draganescu
Copy link
Contributor

@adamziel isn't #24290 actually closing this? Why close it before that gets merged?

@adamziel
Copy link
Contributor Author

@draganescu I saw this issue more as a vehicle to agree on direction, not to track the actual implementation. If it would be useful for you to treat it otherwise, feel free to reopen.

@noisysocks
Copy link
Member

Closing this as #24290 is now merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets
Projects
None yet
Development

No branches or pull requests

3 participants