Widget housekeeping: extend functionality and add check for existing data upon update by AnnaMag · Pull Request #8480 · Automattic/jetpack

AnnaMag · 2018-01-07T19:49:17Z

Side effect of the JPO work on extending the endpoint to inject Business Address widget.

Changes proposed in this Pull Request:

~~remove redundant sidebar placement when updating the widget~~ when updating a widget, it adds a check of whether a widget is in a sidebar before calling a function that moves it there
a helper method to query widget in a sidebar by id_base, not only by widget_id
a helper method to query first active sidebar
a helper method to check if a widget is present in a given sidebar
when updating widget's content we need to check whether such content exists. Currently, the corresponding function (set_widget_settings) is called both when activating a new and updating an existing widget with no checks in place.

Testing instructions:

Jetpack_Widgets have not been used elsewhere, so we do not introduce regression with these changes. UPDATE: outside of https://github.com/Automattic/jetpack/blob/master/json-endpoints/class.wpcom-json-api-add-widget-endpoint.php 😄

follow the instructions in REST API: add functionality for saving Business Address in JPO #8426

Q: Should those changes include separate JP tests?

tyxla

Looks good overall, left some feedback and questions.

Jetpack_Widgets have not been used elsewhere, so we do not introduce regression with these changes.

Note that this is not entirely true, see https://github.com/Automattic/jetpack/blob/master/json-endpoints/class.wpcom-json-api-add-widget-endpoint.php - not sure if that endpoint is used anywhere though. In any case we need to make sure we don't break anything with these changes (if anything is using that endpoint, we should also be testing it). cc @eliorivero

tyxla · 2018-01-08T11:27:39Z

_inc/lib/widgets.php

I know this was like that before, but to me it doesn't make sense for this to be a 500. Should it be a 400 instead?

tyxla · 2018-01-08T11:28:55Z

_inc/lib/widgets.php

This one was useful in case we're moving an existing widget to another sidebar - could you please clarify why we're removing it?

Thanks for reviewing @tyxla! 🙇‍♀️

FWIU, if a widget passed to update_widget is not in the sidebar (also passed as an arg)

$widgets_in_sidebar = self::get_widgets_in_sidebar( $sidebar ); if ( ! isset( $widgets_in_sidebar ) ) { return new WP_Error( 'invalid_data', 'No such sidebar exists', 400 ); }

will return and we would not get to that call anyway. Wouldn't it be clearer to create a separate function e.g. move_widget for those cases? ( I mean in general, not for current iteration of JPO, as we always grab the first sidebar).

Let me know if I misunderstood your question :)

Let me clarify further. The update_widget method is a kind of generic and supports many situations. One of the cases is where the widget is currently in sidebar "x" and you want to move it to sidebar "y" while updating it. Another case is when we want to change its position. In both of these cases, self::move_widget_to_sidebar would move the widget accordingly, but now that you removed that method, it'll no longer handle those cases. Makes sense?

Fwiw, that error message No such sidebar exists does not reflect the actual check 🤔

Good call, worth improving I guess....

☝️ right, agreed. For a moment I read the $widgets_in_sidebar = self::get_widgets_in_sidebar( $sidebar ); as a check whether that specific widget is present.

Two extra points:

the above shows another edge case: if a sidebar is empty and we want to place "our" widget in it, it will return before that happens (if ( ! isset( $widgets_in_sidebar ) ) ). Is this by design?

how about we call the removed line only if required? ( after checking whether the widget is in the sidebar or not)

Makes sense to me, maybe it's best to confirm with @eliorivero who wrote the initial code there.

I added the logic mentioned above and updated the error message.
I still do not see the why behind returning on sidebar with no widgets. It is not a blocker at the moment though.

tyxla · 2018-01-08T11:35:42Z

_inc/lib/widgets.php

Shame we can't use array_filter with an anonymous function here because of PHP version limitations in Jetpack.

tyxla · 2018-01-08T11:36:20Z

_inc/lib/widgets.php

If we alter the return $found to return null and we alter $found = $widget to return $widget, we can omit $found = null at the top.

tyxla · 2018-01-08T11:37:28Z

_inc/lib/widgets.php

With nesting this under the if ( isset( $widget_settings[ $instance_key ] ) ) { check, we'll never see the error if we try to update an incorrect widget?

Check added 👍

The widget is already in the sidebar, so this call is redundant.

`set_widget_settings` is called both on first widget activation and when updating the content. However, we do not distinguish between the two actions and when the content is empty, an error is thrown when accessing widget settings on an empty `instance_key`. This commit corrects that.

Previous commit in the patch removed it assuming that the sidebar already contains the widget to be updated. This will not be the case in general, e.g. when placing a widget in another sidebar. This commit brings it back conditional on the check that the widget is absent from the sidebar. In addition: - it adds a helper function and leaves a couple of comments for clarity. - small code refactor to avoid declaring unnecessary variables - change error code when no sidebar is present - updates error message when no widget is present in a sidebar.'

Adding a check of validity of widget`s ID base for new widgets.

AnnaMag · 2018-01-11T01:42:21Z

Ready for another round of reviews 👍 Thanks!

oskosk · 2018-01-11T13:09:19Z

_inc/lib/widgets.php

+	 *
+	 * @return bool Whether present.
+	 */
+	public static function is_widget_in_sidebar( $widgets_all, $widget_id ) {


❤️ for this method

tyxla · 2018-01-11T15:14:48Z

_inc/lib/widgets.php

+	 * @return bool Whether present.
+	 */
+	public static function is_widget_in_sidebar( $widgets_all, $widget_id ) {
+		foreach ( $widgets_all as $widget_el_id ) {


We should be able to replace this entire foreach with something like:

in_array( $widget_id, $widgets_all, true )

tyxla · 2018-01-11T15:39:08Z

_inc/lib/widgets.php

+		if ( empty( $active_sidebars ) ) {
+			return false;
+		}
+		return array_shift( array_keys( $active_sidebars ) );


This will throw a notice, because array_shift also updates the original referenced array (and there is no reference here).

tyxla · 2018-01-11T15:41:21Z

_inc/lib/widgets.php

+
+	/**
+	 * Find a widget in a list of all widgets retrieved from a sidebar
+	 * using get_widgets_in_sidebar()


It doesn't seem to use get_widgets_in_sidebar()

tyxla · 2018-01-11T15:42:16Z

_inc/lib/widgets.php

+	/**
+	 * Find a widget in a list of all widgets retrieved from a sidebar
+	 * using get_widgets_in_sidebar()
+	 * @param string $widget The widget we want to look up in the sidebar


This doesn't document widgets_all properly.

ockham · 2018-01-11T16:47:10Z

_inc/lib/widgets.php

+	 *
+	 * @return array|false Active sidebar or false if none exists
+	 */
+	public function get_first_sidebar() {


I think we can make this method static, cf. https://github.com/Automattic/jetpack/pull/8426/files#r161009496

AnnaMag · 2018-01-15T11:29:39Z

Closing in favor of #8523.

AnnaMag added [Feature] Extra Sidebar Widgets [Status] In Progress labels Jan 7, 2018

AnnaMag self-assigned this Jan 7, 2018

AnnaMag requested a review from a team as a code owner January 7, 2018 19:49

AnnaMag mentioned this pull request Jan 7, 2018

REST API: add functionality for saving Business Address in JPO #8426

Closed

AnnaMag removed the [Status] In Progress label Jan 7, 2018

AnnaMag requested review from eliorivero, ockham and tyxla January 7, 2018 23:00

AnnaMag added the [Status] Needs Review This PR is ready for review. label Jan 7, 2018

tyxla reviewed Jan 8, 2018

View reviewed changes

AnnaMag mentioned this pull request Jan 8, 2018

REST API: enable widgets module when inserting Business Address widget during JPO #8481

Closed

oskosk added this to the 5.8 milestone Jan 8, 2018

AnnaMag added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jan 10, 2018

AnnaMag added 3 commits January 10, 2018 13:41

Widget: remove redundant sidebar placement when updating the widget

1083497

The widget is already in the sidebar, so this call is redundant.

Widget: enable widget query by id_base.

31afdf1

AnnaMag force-pushed the fix/widget-extend-and-fix-on-update branch 2 times, most recently from 1586368 to 2d33041 Compare January 10, 2018 14:34

AnnaMag added [Status] In Progress and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jan 10, 2018

AnnaMag force-pushed the fix/widget-extend-and-fix-on-update branch from c91e856 to 434ae4a Compare January 10, 2018 23:05

AnnaMag removed the [Status] In Progress label Jan 11, 2018

AnnaMag force-pushed the fix/widget-extend-and-fix-on-update branch from c16fa18 to 04870a1 Compare January 11, 2018 01:22

AnnaMag added 2 commits January 11, 2018 01:24

Widgets: sanitize ID base of widget to be updated.

3ccb162

Adding a check of validity of widget`s ID base for new widgets.

Widgets: add a method to retrieve first active sidebar.

04870a1

AnnaMag requested a review from oskosk January 11, 2018 01:42

oskosk reviewed Jan 11, 2018

View reviewed changes

tyxla reviewed Jan 11, 2018

View reviewed changes

ockham reviewed Jan 11, 2018

View reviewed changes

AnnaMag mentioned this pull request Jan 14, 2018

REST API: add functionality for saving Business Address widget in JPO (alternative) #8523

Merged

AnnaMag closed this Jan 15, 2018

matticbot removed the [Status] Needs Review This PR is ready for review. label Jan 15, 2018

tyxla deleted the fix/widget-extend-and-fix-on-update branch January 15, 2018 11:33

Conversation

AnnaMag commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaMag Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaMag Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaMag Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaMag commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaMag commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AnnaMag commented Jan 7, 2018 •

edited

Loading

AnnaMag Jan 8, 2018 •

edited

Loading

AnnaMag Jan 8, 2018 •

edited

Loading

AnnaMag Jan 8, 2018 •

edited

Loading