[wp-trac] [WordPress Trac] #53816: Overview: Refactor the widgets read/write logic

[WordPress Trac]

#53816: Overview: Refactor the widgets read/write logic
--------------------------+------------------------------
 Reporter:  zieladam      |       Owner:  (none)
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  Awaiting Review
Component:  General       |     Version:
 Severity:  normal        |  Resolution:
 Keywords:                |     Focuses:
--------------------------+------------------------------
Description changed by zieladam:

Old description:

> This is an overview/epic issue that serves as the place to have a
> discussion and also points to many smaller sub-issues.
>
> Widgets-related logic in core got quite confusing over the years. As a
> result, we've dealt with problems such as
> [https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958
> Blocks moving to "Inactive widgets" after saving] (temporarily solved by
> adding an unexpected [https://github.com/WordPress/wordpress-
> develop/pull/1498 wp_get_sidebars_widgets();] call).
>
> There are a few problems there:
>
> * We have multiple global variables with closely related or even roughly
> the same data (`$sidebars_widgets`+`$_wp_sidebars_widgets`, also
> `$wp_registered_sidebars`, `$sidebars_widgets`, `$wp_registered_widgets`,
> `$wp_registered_sidebars`). If we update one, we should also update the
> others for consistency. Sometimes we don't and  we run into
> [https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958
> undefined behaviors].
> * We use a function called `retrieve_widgets` as a mean to fix any
> discrepancies in the stored sidebar-to-widget mapping. It's called
> `retrieve`, but it actually does some writing. This is a source of
> confusion in itself so I [https://core.trac.wordpress.org/ticket/53811
> proposed renaming it].
> * We [https://github.com/WordPress/wordpress-develop/pull/1433 we have to
> call retrieve_widgets in GET API endpoints] which makes it read-write,
> not read-only. This breaks HTTP caching and is also a
> [https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958
> source of bugs].
>
> I don't think we're able to address the proliferation of global variables
> – it seems like a major BC break. However, we should still be able to
> improve the `retrieve_widgets` situation.
>
> Ideally it would:
>
> ☐ Be less monolithic and have a clear, encapsulated flow of logic (as
> suggested by @helloFromTonya)
> ☐ [https://core.trac.wordpress.org/ticket/53811 Have a name suggesting a
> write, e.g. `remap_widgets`]
> ☐ [https://github.com/WordPress/wordpress-develop/pull/1525 Always be
> called **after** a write]
> ☐ Always be called **after** a theme change
> ☐ Never be required to perform a read
> ☐ Never be called in GET request handlers
>
> cc @timotybjacobs @noisysocks @adraganescu @talldan @hellofromTonya
> @desrosj

New description:

 This is an overview/epic issue that serves as the place to have a
 discussion and also points to many smaller sub-issues.

 Widgets-related logic in core got quite confusing over the years. As a
 result, we've dealt with problems such as
 [https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958
 Blocks moving to "Inactive widgets" after saving] (temporarily solved by
 adding an unexpected [https://github.com/WordPress/wordpress-
 develop/pull/1498 wp_get_sidebars_widgets();] call).

 There are a few problems there:

 * We have multiple global variables with closely related or even roughly
 the same data (`$sidebars_widgets`+`$_wp_sidebars_widgets`, also
 `$wp_registered_sidebars`, `$sidebars_widgets`, `$wp_registered_widgets`,
 `$wp_registered_sidebars`). If we update one, we should also update the
 others for consistency. Sometimes we don't and  we run into
 [https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958
 undefined behaviors].
 * We use a function called `retrieve_widgets` as a mean to fix any
 discrepancies in the stored sidebar-to-widget mapping. It's called
 `retrieve`, but it actually does some writing. This is a source of
 confusion in itself so I [https://core.trac.wordpress.org/ticket/53811
 proposed renaming it].
 * We [https://github.com/WordPress/wordpress-develop/pull/1433 we have to
 call retrieve_widgets in GET API endpoints] which makes it read-write, not
 read-only. This breaks HTTP caching and is also a
 [https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958
 source of bugs].

 I don't think we're able to address the proliferation of global variables
 – it seems like a major BC break. However, we should still be able to
 improve the `retrieve_widgets` situation.

 Ideally it would:

 ☐ Be less monolithic and have a clear, encapsulated flow of logic (as
 suggested by @helloFromTonya)
 ☐ [https://core.trac.wordpress.org/ticket/53811 Have a name suggesting a
 write, e.g. `remap_widgets`]
 ☐ [https://github.com/WordPress/wordpress-develop/pull/1525 Always be
 called **after** a write]
 ☐ Always be called **after** a theme change
 ☐ Never be required to perform a read
 ☐ Never be called in GET request handlers

 cc @TimothyBlynJacobs @noisysocks @andraganescu @talldanwp @hellofromTonya
 @desrosj

--

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/53816#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform