Widget visibility: supporting load conditions for legacy widgets in gutenberg widgets screen

[dsas]

Lets the widget visibility conditions run using the new gutenberg based
widgets screen.

Fixes #20057

Changes proposed in this Pull Request:

• Enables visibility conditions to be set on legacy widgets used within the block-based widget editor

Jetpack product discussion

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

• Firstly, use a theme that supports widgets (you probably are already)
• To enable this feature, go to Jetpack → Settings → Writing, scroll down to the Widgets section, and toggle the switch labelled Enable widget visibility controls to display widgets only on particular posts or pages.

Gutenberg widget editor

This requires testing on WordPress 5.8

Widgets admin

• Go to 'Appearance > Widgets'
• Press + inside the editor
• Press visibility, a form should appear
• Set some conditions, e.g. 'Hide if User is Logged Out'
• Press Update near the top of the screen
• Go to the front-end and look at the widget area, the widgets should appear, now logout, it should disappear.

Repeat again, this time set-up two widgets together before pressing update.

Customizer

As above but go to 'Appearance > Customizer' first and use the 'Publish' button rather than 'Update'

Classic widget editor

• Install the classic-widgets plugin
• Test the widgets admin and customizer as above

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello dsas! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D63633-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ⚠️ All commits were linted before commit.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

---

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

---

Jetpack plugin:

• Next scheduled release: July 20, 2021.
• Scheduled code freeze: July 15, 2021.

[dsas]

Current status:

• It displays the visibility form in the new gutenberg’d widget.php + customizer (though it's a bit ugly)
• Ought to work on the old widget experience, though haven’t tried yet
• Doesn’t currently save the conditions
  • Jetpack_Widget_Conditions::widget_update tries to work directly with $_POST but that won't work on the API as the data is not sent via $_POST - so the filter does nothing
  • The filter only receives the updated instance, the previous instance, the newly received instance and the widget. None of these have the conditions info POSTed. The rest API is looking in ['form-data']['widget-foo'][0] for the data to put into instances, however the conditions are in [‘form-data’][‘conditions’].
  • Ultimately, this comes down to the HTML input name.

I think the best way forward is to change the form input names inside the plugin and change Jetpack_Widget_Conditions::widget_update to use the various instance variables rather than using $_POST directly. I think in theory it's fine and backwards compatibility is maintained. widget_conditions_admin receives a widget instance when creating the form so it should be fine.

[dsas]

Remaining:

• Pretty up the form:

Before	After

• Tidy up some remaining $_POST usages (for deciding whether to display visibility open by default?)
• Test changes in customizer
• Test changes in widgets admin for older WordPress
• Test changes in customizer for older WordPress
• Refactor str_starts_with for php8 compat
• Find a different way of identifying when saving using the batch API

[cpapazoglou]

Tested this on WP 5.8 and works as expected both in widgets.php and in customizer.

I have left some comments where I think we can improve the performance, let's discuss them.

[cpapazoglou]

Thanks for refactoring this @dsas! I can confirm that code now runs only when is needed 🎉 !

I tested this on:

• WP 5.8 - Local Jetpack
• WP 5.7 - Simple Site by applying the patch in my Sandbox
• WP 5.7 - Atomic Site by using https://jetpack.com/download-jetpack-beta/ to switch to this branch

It works as expected in all instances!

Let's get the crew in the review process.

[jeherve]

I may be missing something here, but wouldn't that mean that if I click on the site title in the admin bar to access my home page after editing widgets, the widgets would not be filtered then?

[dsas]

This is a very good point :(

The problem I have is that I can't find a way to tell the difference between the API being used to fetch the widgets to display, and the API being used to fetch the list of widgets to edit - it's the same end point - /wp-json/wp/v2/sidebars.

Unless there's something I've missed, the other option is to never filter the list at all.

[jeherve]

That's a good point. I don't have a great solution here either :\

@cpapazoglou Have you had the chance to look at this?

[cpapazoglou]

At first I thought this was a problem with conditions like "Hide if User is logged in" but trying to reproduce it now and the widget still displays after saving them while being in http://localhost/wp-admin/widgets.php.

@dsas is there a chance that this is resolved and we don't need ( false === strpos( wp_get_raw_referer(), '/wp-admin/widgets.php' ) )? Otherwise can you provide the exact steps to reproduce the they could get filtered out and become impossible to edit.?

[dsas]

Yes, it definitely still happens, here are the exact steps, sorry for not providing them earlier:

1. Add legacy widget using /wp-admin/widgets.php
2. Set visibility to "Show if User is logged out"
3. Press update to save them
4. Reload the page
5. See that the newly added widget does not appear.

Screen recording here if that helps: https://d.pr/v/lJVdjv

[dsas]

@jeherve We have solved this by changing the referer check to only be used if using the API.

This means if the API is used it will filter out widgets, unless it's called from the widgets admin page. This means they can still be edited there.

The customizer is unaffected - it doesn't use the same sidebars API anyway.

The solution is perhaps a little bit of a hack, other alternatives we thought about were:

• Never filter out widgets from the API i.e. eliminate the referer check. It didn't seem completely correct and might surprise people. Particularly if someone was using this API for something other than widget editing.
• Get a change upstream so that when the API is called it explains why it's calling e.g. by adding a context=editing request var, we can then look for that request var and choose not to run the filters. This would take some time to coordinate.

[dsas]

Something to note is security. I was worried about depending upon the referer to decide whether or not to filter out conditions, as that's totally in control of the client. The sidebar endpoint is only available to users with the edit_theme_options cap though, so it's ok 👍

[sdixon194]

This works for me! Only thing I noticed was some styling when testing using the customizer (alignment/padding is off for the dropdown menus). Is that something that might be addressed later, or worth looking at now?

[cpapazoglou]

This works for me! Only thing I noticed was some styling when testing using the customizer (alignment/padding is off for the dropdown menus). Is that something that might be addressed later, or worth looking at now?

Presumably we have already addressed these CSS issues. Can you let us know:

• did you run jetpack build?
• which browser are you using?
• are you seeing this in a local Jetpack installation?

[sdixon194]

Ah looks like this occurs only in Firefox (v 90.0) running on my macOS Big Sur 11.4. Works fine for me in Chrome and Safari.

Tested using the Jurassic Ninja live site.

[dsas]

@sdixon194 I can reproduce the select issue (line-height?) in firefox with other selects in other legacy widgets too, even with jetpack plugin disabled.

If you deactivate the jetpack plugin, and then add a navigation menu widget, you should see the same thing.

I'll try to find out where this is coming from, but I don't think it's due to this issue and shouldn't block it.

[sdixon194]

I'll try to find out where this is coming from, but I don't think it's due to this issue and shouldn't block it.

Nice sleuthing! Agreed it's non-blocking! 👍

addressed

[github-actions]

Great news! One last step: head over to your WordPress.com diff, D63633-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

[sdixon194]

Cherry picked to release branch in 4b3956c

[dsas]

Thanks @sdixon194. I checked in the dotorg slack whether this was known and they've now filed WordPress/gutenberg#33431

[sdixon194]

Nice, thanks for checking!

[dsas]

Deployed r228664-wpcom

[georgestephanis]

This seems to be generating some PHP Notices I'm seeing in an error log --

[20-Dec-2021 13:59:32 UTC] PHP Notice:  Undefined index: REQUEST_METHOD in /wordpress/plugins/jetpack/10.5-a.3/modules/widget-visibility/widget-conditions.php on line 87

as the change is doing

if ( 'GET' !== $_SERVER['REQUEST_METHOD'] && false !== strpos( $_SERVER['REQUEST_URI'], '/wp/v2/widgets' ) ) {
	$handle_widget_updates = true;
	$add_html_to_form      = true;
}

without ever checking first to make sure $_SERVER['REQUEST_METHOD'] is defined.

May be worth revising to check if it's not empty() or is isset() first --

https://github.com/WordPress/WordPress/blob/32ecbd061e88c6ffb2ed9ec7cb196908110c2a82/wp-includes/canonical.php#L45

[dsas]

thanks @georgestephanis, might be worth opening up a separate issue with reproduction steps - I can't reproduce it here and want to make sure I've caught all occurrences.

[georgestephanis]

@dsas I've not attempted to duplicate, but my guess is there was a cli script or something being triggered via cli (wp-cron.php maybe) that could lead to the request method not being set.