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

Add Blocks in Widget Areas RFC #14812

Closed
wants to merge 11 commits into from
Closed

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Apr 4, 2019

This RFC outlines the technical approach that we will take to upgrade the widget-editing areas in wp-admin/widgets.php and the Customizer to support blocks.

View the RFC


Fetching and updating the blocks in a widget area is done via new REST API endpoints.

### The widget area resource
Copy link
Member Author

@noisysocks noisysocks Apr 4, 2019

Choose a reason for hiding this comment

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

Unsure what colour to paint this bike shed!

There was some previous discussion in #14612 (comment).

I see three options for the name:

  • Sidebar — this matches what we call these areas in the WordPress Core code, e.g. register_sidebar(), wp_get_sidebars_items(), dynamic_sidebar().
  • Widget Area — this matches what we call these areas in WordPress documentation. It's slightly more accurate since widgets don't have to appear in a site's sidebar.
  • Block Area — this matches what @mapk has been calling these areas in his designs. I do like the idea of getting rid of 'widget' as a term. (What's a widget?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, just saw this and I commented about something similar above. To repeat my thoughts, I think we may have to keep the current terminology as we're still in a transition period and in the future phases, these areas might not be needed anymore and will move to block templates.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I think it's fine if we go with 'widget areas' for now and revisit before merging into Core. The endpoint will be marked as experimental so there's no reason we can't rename in the last minute.

Copy link
Contributor

@mapk mapk Apr 16, 2019

Choose a reason for hiding this comment

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

Are these two different areas going to co-exist during the transition period? Would the user see both "Appearance -> Widgets" and "Appearance -> Block Areas" ? I don't think we should keep the name "Widgets" when we're showing a new interface that allows adding any block, not just widgets.

I also think maybe we can be more proactive here. If the WP install is not using widgets, can we just completely transition over to Block Areas?

I'm also working on a UX flow to help illustrate some of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

In my imagination we would show either "Appearance -> Widgets" if Gutenberg is disabled or "Appearance -> Block Areas" if Gutenberg is enabled. Never both.

)
```

When a widget area stored this way is accessed via the REST API, widgets will be represented using `core/legacy-widget` blocks. Each `core/legacy-widget` block stores a reference to the existing widget (`identifier`), and, if it is a multi widget, the attributes that that instance has (`instance`).
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the alternative here is to have a migration script to avoid touching this site option at all. The cons for a migration script is that if you disable "Gutenberg" or disable the new widgets screen, you won't be able to fallback to the existing widgets screen, you'll lose your areas.

Also, if we migrate the storage, we'll have to rewrite all the php functions dealing with widgets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on this, I think a soft-fallback like you're proposing here is probably the best approach (it kind of maps well with the approach used by the "classic block" in the post_content editor)

Copy link
Member

Choose a reason for hiding this comment

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

A backend based script is the way we should go here. All data would be copied to posts format and not deleted. Copy data shouldn't break anything, until you turn of the new widget screen later.

Copy link
Member Author

Choose a reason for hiding this comment

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

What are the benefits of such an approach?

Copy link
Member

Choose a reason for hiding this comment

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

The backend / rest api is the data layer in this application and should deal with saving data and migrating it. Javascript is the visual layer here and shouldn't have to deal with a complex migration. Also, if you have get the old data, you would have to get that from the api anyway. I think the migration should happen in PHP and javascript should only ever deal with migrated widgets.

There is already a long history of migrations like this happening in the PHP, the team split is a good example. Any migration is going to need to give a action / filter so that PHP develops are right some custom bahavior on the migration if they so wish?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can speak from personal experience that there will be sites with hundreds potentially thousands of widgets. I would insist that a WP CLI based option to upgrade to the new widget storage would be needed.

If that's the case, a PHP migration route would be needed, so why not run it on WP upgrade?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good discussion here! Thanks for hashing out the details.

My understanding is that @spacedmonkey suggests that instead of lazily migrating individual items in sidebars_widgets when a widget area is saved, we ought to eagerly migrate all items in sidebars_widgets when WordPress is installed or when Gutenberg is activated. This means that we don't end up in a scenario where some widget areas are stored different to other widget areas.

I think this makes sense and I'll update the RFC accordingly.

If I understand correctly, then once the migration has happened, all widgets (previously represented in the database as their own entities) will be grouped together into a single post.

Widgets are not currently stored in the database as their own entities—they're serialised into a PHP array which is stored as a site option. Accessing or displaying a single widget is fairly difficult right now.

For backwards compatibility, could we have a "is_legacy_sidebar" parameter for "register_sidebar()" which would prevent the sidebar from automatically being migrated?

@megamenu: Could you elaborate on why you would like this? What's proposed is that existing widgets will work as-is after a WordPress upgrade via the Legacy Widget block.

I think no matter the solution we came up with, it is necessary to be able to go back to the old screen without loosing content.

Definitely, but this is separate to the discussion about whether we migrate sidebars_widgets lazily or eagerly. Either way we will need a way to "un-migrate" the data. See discussion in #14812 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

This RFC keeps mentioning the widgets screen and ignores the customiser. We have been pushing developers to use the customiser for years. Any replacement of widgets, should focus on pushing them to the customiser and hide the old widget screen. I believe there was even a core ticket from @westonruter to hide the old widget screen.

New installs of WordPress should never see the old widget screen either. We need a feature flag in option name, use_legacy_widgets. This will the flag to show the new block areas or not.

This plugin for legacy widgets should filter this value. It should another plugin apart from classic editor. You might want to opt out of widget but keep the new editor.

Copy link
Member Author

Choose a reason for hiding this comment

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

This RFC focuses on the underlying infrastructure which both screens will use. Right now the assumption is that the widget screen is here to stay.

Copy link

@megamenu megamenu Apr 18, 2019

Choose a reason for hiding this comment

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

Widgets are not currently stored in the database as their own entities—they're serialised into a PHP array which is stored as a site option. Accessing or displaying a single widget is fairly difficult right now.

It is not easy (as there is no official api), but it's not impossible to access/display/save a single widget instance. These are the functions (this one and the next 5 or so) that I am concerned about not being able to make compatible/replicate with the proposed changes to how "widgets" (blocks) will be stored:

https://plugins.trac.wordpress.org/browser/megamenu/trunk/classes/widget-manager.class.php?rev=1317340#L512

My plugin allows users to add widgets to their menu. We store all of these widgets within the same sidebar, and pick and choose them individually to output in the correct place. We also provide our own interface for adding and editing these widgets (there is no need for the user to be able to edit those widgets on the widgets.php screen). Video

The "StoreFront Mega Menus" (an official WooCommerce plugin) also stores/displays widgets in a similar way, although they have completely hidden their own sidebar from the widgets.php screen. I suspect there are other plugin/theme authors who do similar things who have not seen this RFC yet.

[r.e. is_legacy_sidebar suggestion) Could you elaborate on why you would like this? What's proposed is that existing widgets will work as-is after a WordPress upgrade via the Legacy Widget block.

I'm concerned it will be almost impossible to make my plugin compatible with the proposed changes, due to the shift from a sidebar being built up from of a collection of individual entities (widgets), to a sidebar basically being saved/stored/represented as one big content area.

I suggested the "is_legacy_sidebar" option because if there will be core support (albeit exposed via a plugin) for "legacy" sidebars anyway, I will not have to ask my users to install the "Legacy Sidebars" plugin. I can simply mark the sidebar as "legacy" (and even hide it from the widgets.php screen). It would also mean users would not have to convert all of their sidebars back to "legacy" sidebars just to use my plugin - we could have some sidebars that are "legacy" and some that are "block enabled".

Then again, maybe I am not fully understanding correctly? If I just hid my sidebar from the widgets.php screen, could I expect the plugin to work "as is" given the proposed changes?


```php
array(
'footer' => 123,
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if I downgrade gutenberg or disable this new screen (which I think will be needed for at least some time).

Can't we instead do something like:

array(
   'footer' => array( 'block-area-widget' ) // instance widget with the id 123

This is to say, instead of chaning the format, just keep the same and have a "hidden" widget called "block-area-widget" referencing a wp_area record.

The advantage is that all the php functions will work as expected, disabling this new behavior ensures a decent fallback.

Copy link
Member

Choose a reason for hiding this comment

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

@youknowriad With the current approach @noisysocks is proposing, we can also hook into the sidebar areas and register a dynamic callback widget that displays the blocks saved the post that sidebar references as a widget in the widget area so we can be back-compatible.
In PR #14251 @noisysocks is storing all the blocks as a string but then he dynamically parses the blocks and registers each one so each block appears in the widgets screen as a widget, here we could something similar.

I guess in the approach you are suggesting we would save the ids as instance attributes of the widget in the database. That approach has some performance disadvantages compared to this version.

We would first need to make a database query in the site options database, wait for the query result decode the widget instance and then make another query for the post id. In this approach, we just need to query the post id. So fewer queries to do.

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'm hesitant about relying on "tricks" like this to ensure backwards compatibility. We're changing the data structure and shouldn't shy away from that. I'd rather that WordPress spits out a loud mess of PHP notices than for a subtle bug to be missed.

We can support disabling the new screen as @jorgefilipecosta nicely explains above.

We could support disabling the Gutenberg plugin by registering a deactivation hook that does clean up and replaces the blocks in a widget area with a Text widget.

Supporting downgrading is trickier. Is it a requirement? I didn't think that WordPress ever supports being downgraded to an older version. Usually one restores from a backup instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we document the proposed solution here in the RFC.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 7f59b59.

Copy link
Member

Choose a reason for hiding this comment

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

Reading this again in consideration of the "migration", I too had become curious if there could be some option where we don't need to change the structure. In my mind, I'd wondered if the structure could be left the same for existing widgets, and as new blocks content is added, doing so either as some custom widget, or perhaps even as the existing "Custom HTML" widget.

Considering some "Footer" with a search widget:

array(
	'footer' => array(
		'search-2',
		// {"title":"Search for content:"}
	),
);

When presented in the widgets screen, the data would either be sent as if it were blocks (core/legacy-widget) or as a widget, but interpreted on-the-fly to an equivalent legacy widget block in the client.

The interaction would feel the same as any other block editor. When ultimately saved, it might either send as blocks markup, or as the equivalent widgets data. In either case, what ends up being stored in the server aligns to this existing structure.

array(
	'footer' => array(
		'search-2',
		// {"title":"Search for content:"}

		'custom_html-2', // or 'block_html-2', for semantic awareness
		// {"content":"<!-- wp:paragraph -->...<!-- /wp:paragraph -->\n\n<!-- wp:image -->...<!-- /wp:image -->"}
	),
);

This seems like the direction that #14251 had been heading with its "block widget". It's not clear the reasons it was pivoted toward an entirely new structure here.

It does depend on a few factors, however:

  • How simple is this "migration" to consider? It seems like a common point of contention for both timing and backwards-compatibility. If we could avoid it, that becomes moot.
  • Is this idea of an "area" post type meant to satisfy some long-term objective (e.g. "content areas" from 9 Projects for 2019), or merely as compatibility for widgets?
  • Are we intent on abolishing the typical storage mechanism of widgets as an intentional consolidation of blocks markup as the storage mechanic?
    • Data consumed by the widgets screen would presumably either arrive from a widgets endpoint (which contradicts this goal to relegate widgets to legacy) or by some endpoint specific to this purpose (i.e. probably something like the "areas" just like what you're proposing). This could dismiss part of my worry; or not, if we grant that an areas endpoint should serve blocks markup, but that this be computed on the fly without changing the existing structure for widgets.

I'm not trying to shake things up dramatically at this stage, but in reviewing #15014 it had me considering what even is an "area"? And all of the effort we're putting in toward migrating storage.

Copy link
Member Author

@noisysocks noisysocks May 9, 2019

Choose a reason for hiding this comment

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

I do like the idea of minimising effort here, especially if we have a view to making widgets redundant in the medium to long term.

An approach involving something akin to your block_html widget was the first I tried during prototyping. I ran into difficulties implementing the PUT endpoint as it requires an algorithm that doesn't unnecessarily create and delete existing widgets. Let me do some experimenting and get back to you.

Copy link
Member Author

@noisysocks noisysocks May 10, 2019

Choose a reason for hiding this comment

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

@jorgefilipecosta and I spoke about this and noted:

  • Neither approach is really "simple". With CPTs the complexity is in migration and backwards compatibility. With HTML widgets the complexity is mostly in updating a widget area.
  • Using CPTs has some scalability advantages noted in #35669-core.
  • wp_area gives us flexibility in the future to have post revisions and possibly to implement "content areas". You could imagine that a "content area" is just a block that points to one or more wp_area IDs.

We agreed to continue with the wp_area approach. As with all things Gutenberg, though, experimentation and iteration is necessary and I'm comfortable revisiting this before merge if need be.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Awesome work on this RFC @noisysocks I think this is a great start :) I left some comments


## Performance

`wp_area` posts will be pre-emptively fetched when rendering a page. That is, all of the posts referenced in `'sidebar_widgets'` will be fetched using a single `SELECT ... FROM wp_posts WHERE ID in ?` query. This ensures that we do not introduce one extra query per widget area.
Copy link
Member

Choose a reason for hiding this comment

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

Currently on widgets areas are autoloaded on every page. Which is a currently issue. All wp_area posts would need to load on every page, as you just never know what widget areas are displayed on each page.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. It's tricky to know ahead of time which widget areas will be used, though. We'd have to (ab)use output buffering or some such.

I think that this is an optimisation we should look at doing separately in the future.

Copy link
Member

Choose a reason for hiding this comment

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

We have two ways of going about this, either always every wp_area post on every request, or just load them as they are requests. So one big request or lots of little ones. Lots of little ones makes more sense, say some pages don't have widgets on them at all, like say a 404 page...

Copy link
Member Author

Choose a reason for hiding this comment

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

What's more common: That a template uses all of the sidebars registered by the theme, or that it doesn't?

Copy link
Member

Choose a reason for hiding this comment

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

There is no common way of using widgets areas, it is just unknownable. I feel like we are going to have load all widget areas on every request, which I hate but I dont know if they there is anyway of knowing which widget areas in use at anytime.

@gziolo gziolo added [Type] RFC [Feature] Blocks Overall functionality of blocks [Feature] Widgets Screen The block-based screen that replaced widgets.php. labels Apr 4, 2019

### The widget area resource

The REST API is built around a single resource object: the widget area.
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 this need another API again? If the data is just wp_posts, why can't it use the existing apis with some extra fields added, similar to the attachments / media endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the main differences are:

  • this REST API doesn't allow creating widget areas as these are defined by themes.
  • It supports fallback (migration) to the old storage format.
  • It has a custom behavior to handle the "inactive" widget area.

Copy link
Member Author

Choose a reason for hiding this comment

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

@youknowriad is correct. Unlike posts, widget areas can't be created or deleted.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't a widget area now map to a post of type wp_area? How is that post created if you don't create method. API, should have create methods, it is a full crud api, even if this method isn't directly used.

It could use the basics of the post controller and override it methods as required.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would POST /wp/v2/widget-areas do? It can't call register_sidebar() on behalf of the theme.

My thinking is that wp_area is an implementation detail and that, as far as consumers are concerned, widget areas are a resource that can be read and updated but not created or deleted.


After migration, the widget area identifier no longer maps to an array of widget identifiers. Instead, it maps to the ID of a `wp_area` post.

`wp_area` is a new post type that denotes posts that contain block markup for a widget area. The `wp_area` post type is similar to the `wp_block` post type, except that it is marked as private and therefore not viewable by accessing `wp-admin/edit.php?post_type=wp_area`.
Copy link
Member

Choose a reason for hiding this comment

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

Will this new post type support revisions? How about authors?

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting! revisions could possible be good to have but I guess we should aim for the minimal solution first and iterate on enhancements.

Copy link
Member

Choose a reason for hiding this comment

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

We could leave them turned on, with new ui and then plugin developers could add this as a feature project.

Copy link
Member

Choose a reason for hiding this comment

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

Currently supporting previews in the customizer where it shows new versions of widget instances with changes but does not really saves them until a publish is really complex. Having the concept of published block area and draft block area may be very useful. During the previews, we use the last drafts of the block areas while still using the published version on the normal view.

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 for now, no, but part of what persuaded me to use wp_area posts is that it steers us towards a future where all sections of your site are version controlled and support publishing flows.

Copy link
Member

Choose a reason for hiding this comment

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

This is how the customizer changesets were created. They use post type, becuases you have the ability to draft and send previews to others using a special link because revisions. If we don't turn them on from the start, we will use that data forever.


## Performance

`wp_area` posts will be pre-emptively fetched when rendering a page. That is, all of the posts referenced in `'sidebar_widgets'` will be fetched using a single `SELECT ... FROM wp_posts WHERE ID in ?` query. This ensures that we do not introduce one extra query per widget area.
Copy link
Member

Choose a reason for hiding this comment

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

Are there performance issues around storing many widget / blocks in one post like this? How about each widget is a single post with a single block and they are groups with a taxonomy?

Copy link
Member

Choose a reason for hiding this comment

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

Or a widget area is a wp_area post and the widgets are all reusable blocks?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these two things wp_area and wp_block (reusable blocks) are two separate things with two different use-cases. They share technical aspects though, they have the exact same type of content (list of blocks), they can be both considered "templates" (in the broad sense, not WP one)

Copy link
Member

Choose a reason for hiding this comment

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

Okay, agree regarding reuseable blocks. But what about my point on lots of blocks in one post? Some sidebars have tonnes of blocks. Worried about this scaling.

Copy link
Contributor

Choose a reason for hiding this comment

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

How is this different from a regular post/page containing tonnes of blocks?

Copy link
Member

Choose a reason for hiding this comment

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

How is this different from a regular post/page containing tonnes of blocks?

Yeah, I don't like them either, they don't scale great, but we couldn't develop it different without breaking BC. With this, we have more options.

Copy link

Choose a reason for hiding this comment

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

I would personally like to see widgets stored individually in their own posts. It makes sense for multiple blocks to be stored together when writing a post or page, but imho widgets are distinct pieces of content and should continue to be stored that way. The current method of storing widgets is not ideal, but it does at least allow us to fairly easily extract or save a single widget instance. That doesn't seem so possible if all widgets within a sidebar as lumped together.

Storing them individually would also open up the ability to add metadata to a widget, something that until now has required 'hacking' the options table.

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering widgets will move at some points to blocks entirely @megamenu would saving each widget individually be still needed?

Choose a reason for hiding this comment

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

@draganescu I understand the core widgets all have their own block equivalent, and at some time maybe those core widgets will be gone, but I'm assuming there will always be some form of support for custom "legacy" widgets?

Copy link
Member Author

@noisysocks noisysocks Apr 18, 2019

Choose a reason for hiding this comment

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

Perhaps @youknowriad can speak better to this, but I'm drawn to storing multiple blocks per widget area because it fits nicely with my mental model of how WordPress will look in the future which is that one's site will consist entirely of block areas. In this sense, the similarities between post_content, wp_block and wp_area are intentional.

+--------------------------------------------------------------------------+
|                                                                          |
|                             "header" wp_area                             |
|                                                                          |
+--------------------------------------------------------------------------+
+-----------------------------------------------+  +-----------------------+
|                                               |  |                       |
|                                               |  |                       |
|                                               |  |                       |
|                                               |  |                       |
|                                               |  |                       |
|                                               |  |                       |
|                                               |  |                       |
|                 post_content                  |  |                       |
|                                               |  |                       |
|                                               |  |   "sidebar" wp_area   |
|                                               |  |                       |
|                                               |  |                       |
|                                               |  |                       |
|                                               |  |                       |
|                                               |  |                       |
|  +-----------------------------------------+  |  |                       |
|  |                                         |  |  |                       |
|  |           "author bio" wp_block         |  |  |                       |
|  |                                         |  |  |                       |
|  +-----------------------------------------+  |  |                       |
|                                               |  |                       |
+-----------------------------------------------+  +-----------------------+
+--------------------------------------------------------------------------+
|                                                                          |
|                                                                          |
|                                                                          |
|                             "footer" wp_area                             |
|                                                                          |
|                                                                          |
|                                                                          |
+--------------------------------------------------------------------------+

docs/rfcs/blocks-in-widget-areas.md Show resolved Hide resolved
docs/rfcs/blocks-in-widget-areas.md Outdated Show resolved Hide resolved
docs/rfcs/blocks-in-widget-areas.md Show resolved Hide resolved

```php
array(
'footer' => 123,
Copy link
Member

Choose a reason for hiding this comment

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

@youknowriad With the current approach @noisysocks is proposing, we can also hook into the sidebar areas and register a dynamic callback widget that displays the blocks saved the post that sidebar references as a widget in the widget area so we can be back-compatible.
In PR #14251 @noisysocks is storing all the blocks as a string but then he dynamically parses the blocks and registers each one so each block appears in the widgets screen as a widget, here we could something similar.

I guess in the approach you are suggesting we would save the ids as instance attributes of the widget in the database. That approach has some performance disadvantages compared to this version.

We would first need to make a database query in the site options database, wait for the query result decode the widget instance and then make another query for the post id. In this approach, we just need to query the post id. So fewer queries to do.


After migration, the widget area identifier no longer maps to an array of widget identifiers. Instead, it maps to the ID of a `wp_area` post.

`wp_area` is a new post type that denotes posts that contain block markup for a widget area. The `wp_area` post type is similar to the `wp_block` post type, except that it is marked as private and therefore not viewable by accessing `wp-admin/edit.php?post_type=wp_area`.
Copy link
Member

Choose a reason for hiding this comment

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

Currently supporting previews in the customizer where it shows new versions of widget instances with changes but does not really saves them until a publish is really complex. Having the concept of published block area and draft block area may be very useful. During the previews, we use the last drafts of the block areas while still using the published version on the normal view.

@aduth
Copy link
Member

aduth commented May 16, 2019

In yesterday's #core-editor meeting, I raised a worry that this pull request in its current form is no longer productive for coming up to speed with discussion and for following developments, and to the general point of allowing for RFCs to be merged as working documents and iterated upon pending initial implementations.

https://wordpress.slack.com/archives/C02QB2JS7/p1557927307328700 (link requires registration)

Out of this discussion, I also proposed the following pull request which is intended to outline the process and clarify how a status should be communicated, and how interested parties should be expected to follow along with progress:

#15687

I'd encourage that we try to merge this pull request as soon as it's been updated to reflect the current state of the discussion (now, if that's already the case), and allow for iterations on specifics of the document in subsequent pull requests.

For this RFC, I've created a column in the Request for Comments (RFCs) project to follow along:

https://github.com/WordPress/gutenberg/projects/26#column-5381044

As I've not been heavily involved in the discussion thus far, I don't feel I'm personally in a position to be able to approve this.

@spacedmonkey
Copy link
Member

Why would this be merged, I and other have highlighted many many issues with it. What is in this document seems unworkable and will likely have negative effects on developers and users. Until this feedback is actioned we have answered to these issues, please do not close this PR.

@aduth
Copy link
Member

aduth commented May 16, 2019

Why would this be merged, I and other have highlighted many many issues with it. What is in this document seems unworkable and will likely have negative effects on developers and users. Until this feedback is actioned we have answered to these issues, please do not close this PR.

Did you read through the links in my previous comment? The reasons are outlined at depth in the conversations and resulting process pull request. It's important to not consider this as final, to communicate its draft status clearly, and to provide guidelines for how it should progress and how contributors can follow along and continue to provide feedback.

Just like it would be unwise to attempt a single pull request implementing the entirety of a block-based editor or widgets screen, we should embrace iteration here. Otherwise, momentum stalls, participation is discouraged, and we will devise a subpar proposal by virtue of the unknown unknowns (that which we can't know until early implementations provide the information necessary to refine the proposal).

@spacedmonkey
Copy link
Member

I did read it and if I had attended that meeting I would have disagree there. This PR shouldn't be merged, it should be closed and a new RFC written, taking the feedback given to date. Merging it sends the wrong message, that this RFC is some how approved, where it isn't as it only is approved by members of the of the gutenberg team. Everyone outside of that team that has comments has disappointed this RFC.

This is now the 3rd or 4th PR I have comments on without workable answered to my feedback. At some point you are just ignoring my feedback and that is not how any open source project should be run.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

This RFC got deeply discussed; we are trying the proposed path. Given the number of discussions, it is hard to continue the talks in this PR.
I think this PR is ready to be merged, that does not mean the RFC is finalized. Further improvements and changes are welcome and can be done in separate PR's that focus on a specific change/topic. Having specific PR's makes the discussion more manageable.
Thank you @noisysocks for starting this RFC and thank you to all that provided feedback and contributed to the discussion.

@aduth
Copy link
Member

aduth commented May 16, 2019

@spacedmonkey First and foremost, I see this as a matter of project management, and not a commentary about this specific RFC. Any pull request exceeding 180 comments has outlived its usefulness, and is a symptom of a flawed process. This is the point I raised for discussion in yesterday's meeting, and for which I've proposed improvements at #15687. I agree with you that messaging is critical, and I've outlined specific recommendations for how statuses can be communicated most effectively. Your feedback would be appreciated there. More than that, it aims to keep these conversations both focused and based in fact, informed by real-world application of a draft.

While I don't agree that reopening this pull request would be the most effective way to continue this feedback loop, I feel we should direct this discussion of process to #15687. In retrospect, I think it'd be fair to say that merging this pull request should be contingent upon approval of that revision in process.

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented May 16, 2019

I did read it and if I had attended that meeting I would have disagree there. This PR shouldn't be merged, it should be closed and a new RFC written, taking the feedback given to date. Merging it sends the wrong message, that this RFC is some how approved, where it isn't as it only is approved by members of the of the gutenberg team. Everyone outside of that team that has comments has disappointed this RFC.

Hi @spacedmonkey, when we merge code, it does not mean that code will be final, further changes and iterations may happen, and that code may even end-up deleted. Usually, to implement a feature, we have several PR's to make sure each PR is not huge, and each line of it gets the deserved attention.

Although an RFC is not code, it suffers from the same problem; it is tough to get it right in single PR, new comments and changes here will probably not get the deserved attention.

Following the same approach, we do for code, where we interactively improve things, and we avoid bigger PR's, seems a good option.

In a future RFC, I can even imagine that the first PR contains the sections, and then for each part, we have a separate PR.

Merging this RFC does not mean it is final. Future enhancements will probably be proposed, and anyone is free to suggest changes and improvements in a follow-up PR. It is easier to discuss changes/alternatives when they are materialized in a PR.

@spacedmonkey
Copy link
Member

Any pull request without 180 comments and no resolution should be merged. That is not sane. It is clear this rfc has problems. Merging it sends the wrong message. We should wait until word amp Europe, meet in person and discuss this. After that, we can write a rfc together.

Merging this sends a horrible message to the community and stops others from feedback in the future.

@aduth
Copy link
Member

aduth commented May 17, 2019

@spacedmonkey Let's keep discussion of process centralized to #15687, where I've invited others to provide feedback as well. I'd welcome suggestions on how the messaging can be made clearer.

WordCamps can be a great venue for discussion, but we shouldn't hinge a decision on the outcome of a single event, since it's exclusionary to those unable to attend. That said, I do think it's entirely compatible with the goals of the revised process of #15687 that as a result of said discussions, we could devise refinements to a working draft of this proposal.

@spacedmonkey
Copy link
Member

My biggest issue here, is that the "RFC", there are lots of assumptions in this documents. Many of which are not compability with WordPress's previous projects and backwards compatibility. This PR, doesn't mention other ways of running this migration / project, that might be less harmful to users. Say for example, adding a theme_supports to functions file to allow users to opt in block areas. Then we can slowly migrate users over a couple of versions.

The problem here, is this project needs scope and exceptions. Do we exception users to be able to rollback to the old widget screen without loosing data. We need align on this, which a meeting in personal will massively help. I know not everyone can attend, but that is the same with slack meetings, as long as minutes are taken and published, it should be fine.

@tomjn
Copy link
Contributor

tomjn commented May 18, 2019

As it stands, this PR contained too much stuff, and now that people have commented, it's near impossible to get the gist of what's going on without blocking out a day to read and re-read both the PR and all the comments and discussions

I believe it was a mistake to include migration in the original RFC, migrating should be a separate project discussed with its own merits, especially since there may be multiple methods of doing it that suit different contexts.

For the future, it would be better to break things apart into sub-projects, and avoid ambiguous names such as RFC ( and accronyms altogether ). A consultation, or draft proposal come to mind.

The problem here, is this project needs scope and exceptions. Do we exception users to be able to rollback to the old widget screen without loosing data. We need align on this, which a meeting in personal will massively help. I know not everyone can attend, but that is the same with slack meetings, as long as minutes are taken and published, it should be fine.

I agree an in person meeting would be useful, we're all commenting here and if our opinions and thoughts are well laid out, it shouldn't matter as much that they were informed by in person discussions.

The problem comes when those in person meetings are used to make decisions.

@aduth
Copy link
Member

aduth commented May 20, 2019

I think it might help if we could specifically identify:

  • What are the current points of disagreement?
  • What specific follow-up steps can we devise to ensure that they are appropriately discussed and addressed?

Upon identifying these:

  • Can we add specific clarifying notes to the document itself, mentioning the instability of the proposed writing?
  • Can we create corresponding issues and/or cards to include on the project board column?

For the future, it would be better to break things apart into sub-projects, and avoid ambiguous names such as RFC ( and accronyms altogether ). A consultation, or draft proposal come to mind.

I agree. In the later revisions of #15687 and considering how other projects implement similar workflows, the status is a key component of what it is and how it's presented (W3C "Drafts" vs. "Recommendations", TC39 "Stages"). While "RFC" might not be strictly inaccurate use of the term, I grant there's some ambiguity to it, leading to interpretations of its finality which is (as I understand it and in reflection of its execution here) not the intention.

@spacedmonkey
Copy link
Member

By is it a problem that this PR has 187 comments, when this RFC has 136 comments (at time of writing)?

@aduth
Copy link
Member

aduth commented May 24, 2019

It's a problem for both, which is why it was presented as an issue of the process, not of the specific proposals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Widgets Screen The block-based screen that replaced widgets.php.
Projects
None yet
Development

Successfully merging this pull request may close these issues.