Add show show_in_rest param to register_sidebar

[spacedmonkey]

Sidebars can be queries via the sidebar REST API, added in WordPress 5.8. However this data is not public, as requires the current user to be have rights to edit the widget to see the sidebar in the api. Sidebars are registered in PHP, so adding show_in_rest param, should be easy. This opt-in feature would make sidebar data, including embedded widgets, public, if the dev opts in.

[TimothyBlynJacobs]

I think this makes a lot of sense to add. Feel free to milestone for 5.9.

Trac ticket: https://core.trac.wordpress.org/ticket/53915

[spacedmonkey]

Thanks @TimothyBlynJacobs

I have a PR at ​#1578.

It is a surprisingly simple change.

Won't we need a change in the widgets controller to check if a widget is part of a public sidebar?

@TimothyBJacobs Updated the PR with your feedback.

One question. Should we / can we change raw instance values to only display in edit context.

​https://github.com/WordPress/wordpress-develop/blob/eb6b517aec3e04b1868a4ea4e4eb01a6a08a3751/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php#L783-L787

I am wonder about sensitive data leaking?

Thanks. Looks like the test failure may be legit.

We would need to change the whole instance value to be only be exposed in an edit context, since the encoded form is just encoded, not hidden.

I think sensitive data leakage is definitely something we should be concerned about. While a theme author may opt-in to having their sidebar available, they might not know that a widget's setting contains an API key for instance.

However, I think there is definitely value in having the instance values available, not just the rendered widget, to allow for rendering the widget yourself.

I think for now we could omit the instance entirely, but it may make sense in the future to allow for show_instance_in_rest to be overloaded to a configuration array similarly to register_meta to allow for a widget author to selectively expose certain fields.

Seems like the issue here was running retrieve_widgets twice, once in permission check and once in *_items function. I added a workaround for this, with some state is seems to work. I would love some feedback on managing state like this better. Thoughts @TimothyBJacobs

Pinging @adamziel as he's been looking into retrieve_widgets issues.

We would need to change the whole instance value to be only be exposed in an edit context, since the encoded form is just encoded, not hidden.

So should only instance be changed to 'context' => array( 'edit' ), or instance, instance.encoded, instance.hash and instance.raw be changed?

​https://github.com/WordPress/wordpress-develop/blob/52fe9ffbd59955c30e1e91ab0ba0f04e72243d8d/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php#L767-L789

I added a workaround for this, with some state is seems to work. I would love some feedback on managing state like this better.

@spacedmonkey I think the mental model of `retrieve_widgets` is broken right now. The name is confusing, that function doesn't just retrieve stuff, it actually updates the database. It's like "switching themes breaks the stored data, so we need to fix it every time we use it". I would like to make it more "we never break the data in the first place" but that's a larger project. For now I just ​proposed a more accurate name and it has commit label so you might want to update this PR once that change is merged.

Since your issue seems to come from running that function twice and ​we've seen similar problems, I wonder if ​this fix might help you.

Other than that, I don't think that we have a good way around running that function multiple times like you do now. Well, maybe aside of maybe a hook that would run when a route is matched – that way we could have only a single call that would cover both the permissions check and the actual handler.

@adamziel Do you think that ​https://github.com/WordPress/wordpress-develop/pull/1525 blocks this PR? Would it effect anything in your PR? Did you review this PR?

l Do you think that #1525 blocks this PR?

@spacedmonkey I wouldn't say it blocks, no. Maybe it would allow you to get rid of this check:

{{{php

if ( $this->widgets_retrieved ) {

return;

}

}}}

But I'm not 100% certain about that since I don't have a good grasp of what the problem was.

Would it effect anything in your PR?

I don't this PR would affect #1525 much – a rebase, if necessary, should be pretty easy. Feel free to move forward with this one once you have approvals.

Did you review this PR?

I read the code and it looks good, but I did not test much. I'm happy to help with testing once you confirm the code is where you want it to be, sync_registered_widgets or not.

Also, one more question – should this PR be moved to the Gutenberg repo so that 5.7+plugin may benefit from these changes? That's what we did with ​https://github.com/WordPress/gutenberg/pull/34230, it may or may not make sense here too

Also, one more question – should this PR be moved to the Gutenberg repo so that 5.7+plugin may benefit from these changes? That's what we did with WordPress/gutenberg#34230, it may or may not make sense here as well

Yeah, it can be backported.

I wouldn't say it blocks, no. Maybe it would allow you to get rid of this check:

So we should wait until your work is merged before this goes in?

If it takes a few days then it would be convenient – potentially one check less in your PR. If it takes more, it's probably not worth it – we could re-evaluate that check at some later time.

Yeah, it can be backported.

Why not Gutenberg first? It would be picked up during the next major WP version as a part of the regular workflow. See the discussion in ​https://github.com/WordPress/gutenberg/issues/33810

Yeah, it can be backported.

Why not Gutenberg first? It would be picked up during the next major WP version as a part of the regular workflow. See the discussion in ​https://github.com/WordPress/gutenberg/issues/33810

Yeah, it can be backported.

Why not Gutenberg-first? These "double" changes have been challenging lately, see the discussion in ​https://github.com/WordPress/gutenberg/issues/33810 and ​https://github.com/WordPress/gutenberg/pull/34536.

The sidebars endpoint is getting removed from the next Gutenberg version, so no problem there. It's all about having a consistent widgets endpoint now.

Why not Gutenberg-first? These "double" changes have been challenging lately

This change doesn't make sense to backport. It has little use in the gutenberg plugin and it an opt in feature for devs. It is most useful for headless applications of WordPress.

This change doesn't make sense to backport. It has little use in the gutenberg plugin and it an opt in feature for devs. It is most useful for headless applications of WordPress.

I see, thank you for explaining! I am only concerned about merging changes later on. If we keep updating the same endpoints both in core and in the Gutenberg repo, we may end up with conflicting changes. On the other hand, I think I discussed that with @gziolo in ​https://github.com/WordPress/gutenberg/issues/33810 and the conclusion was that in practice this rarely becomes a problem.

this rarely becomes a problem so maybe we're good here?

If there was any value in adding this to the gutenberg plugin, I would have done it both places. I will do that future PRs.

Unit tests are passing!

PHPUnit failures seem to be just flaky CI. I think this PR is pretty close 👍 I left some last notes and proposed a few tests.

[TimothyBlynJacobs]

In 52016:

REST API: Allow sidebars and their widgets to be public.

By default, only users with the edit_theme_options capability can access the sidebars and widgets REST API endpoints. In this commit, A new show_in_rest parameter is added to the register_sidebar function. When enabled, all users will be able to access that sidebar and any widgets belonging to that sidebar.

This commit reduces the context for a widget's instance information to only edit. This is to ensure that internal widget data is not inadvertently exposed to the public. A future ticket may expose additional APIs to allow widget authors to indicate that their instance data can be safely exposed. REST API consumers intending to access this instance information should take care to explicitly set the context parameter to edit.

Props spacedmonkey, zieladam.
Fixes #53915.

Merged in https://core.trac.wordpress.org/changeset/52016.