Display Posts Widget: speed and stability improvements. #3207

bisko · 2015-12-24T09:25:02Z

This Pull Request contains speed and reliability improvements of the Display Posts Widget.

The following things have changed since the previous version:

Blog information and posts are collected in the background through wp_cron, so there would be no page load blocks when the cache expires and new information is pulled.
Widget instance information is now unified and stored permanently in an option and transients usage has been dropped from the main widget functionality.
Cache storage size has been greatly reduced.
Widget load and render speed has been improved and made more consistent.
Errors are now more informative and are shown in the widget configuration.
Errors shown to visitors in the widget area have been reduced. If we have blog posts cached, we always show the last copy we have, not an error.
Some general code improvements and HTML fixes.

mdawaffe · 2016-01-05T23:24:39Z

modules/widgets/wordpress-post-widget.php

+ * Add a 10 minute interval to cron intervals.
+ */
+add_filter( 'cron_schedules', 'jetpack_display_posts_widget_cron_intervals' );
+function jetpack_display_posts_widget_cron_intervals() {


This looks wrong. A filter should accept a value, modify (or replace it), and then return it.

What would happen if another plugin were adding a minutes_30 interval via the cron_schedules hook?

All previously set custom intervals would have been rewritten if left as such.

I updated the method (34df2dfd43e26d520e5a09c8b441913a921bedfc) to work as expected and not overwrite the schedules.

mdawaffe · 2016-01-06T00:04:34Z

Jetpack uses cron in a couple other places. If the Jetpack plugin is disabled (or if all the Display Posts Widgets are removed from the active sidebars), it'd be nice if the this widget's cron task were unscheduled.

See, for example, Jetpack_Heartbeat::deactivate() and it's use at

jetpack/class.jetpack.php

Line 3032 in 6474a65

Jetpack_Heartbeat::init()->deactivate();

mdawaffe · 2016-01-06T00:06:57Z

Can you make sure the brace style is correct in this code?

mdawaffe · 2016-01-06T00:11:55Z

modules/widgets/wordpress-post-widget.php

+		/**
+		 * No body has been set in the response. This should be pretty bad.
+		 */
+		if ( ! isset( $service_response['body'] ) ) {


It's better to use wp_remote_retrieve_body() to access this.

Updated it in 5a546e02815c75fab298b59651542d055e52e713.

mdawaffe · 2016-01-06T00:21:24Z

What will happen if a site already has one (or more) of these widgets active in their sidebar with the current version of Jetpack and then upgrades Jetpack. Will the widget still work?

mdawaffe · 2016-01-06T00:36:27Z

I suspect this has always been a problem, but:

If I leave the "Blog URL" field blank in the widget's form, there's no error message in the form, and the widget just always says "Cannot load blog information at this time." in the sidebar.

mdawaffe · 2016-01-06T00:38:39Z

modules/widgets/wordpress-post-widget.php

+			 * Prepare the error messages.
+			 */
+
+			$where_message = __( 'An error occurred while downloading ', 'jetpack' );


It's always best to put the full sentence/phrase/thought into a single translation call. Can we rephrase this/these somehow?

I updated the code relating to that in 9a154e831b5823626dc787b780607b06a84a5579. Is this a better way to handle such situation?

bisko · 2016-01-07T09:38:42Z

What will happen if a site already has one (or more) of these widgets active in their sidebar with the current version of Jetpack and then upgrades Jetpack. Will the widget still work?

The widgets will still work after the update as there were no changes to the configuration format and how it is saved.

There will be a bit of time after the update that the widgets will show an error, as the cron needs to be run at least once, in order to gather data for the widget and store it in the new format. Is this a good trade-off?

bisko · 2016-01-07T10:41:16Z

I suspect this has always been a problem, but:

If I leave the "Blog URL" field blank in the widget's form, there's no error message in the form, and the widget just always says "Cannot load blog information at this time." in the sidebar.

Yes, no error was shown if there was no URL specified. I added an error message in 1795ca3de8a394400081a98003e77ce625ff4767.

bisko · 2016-01-07T17:00:24Z

Jetpack uses cron in a couple other places. If the Jetpack plugin is disabled (or if all the Display Posts Widgets are removed from the active sidebars), it'd be nice if the this widget's cron task were unscheduled.

See, for example, Jetpack_Heartbeat::deactivate() and it's use at

jetpack/class.jetpack.php

Line 3032 in 6474a65

Jetpack_Heartbeat::init()->deactivate();

I added functionality (in c3e1fc28771ccd626357984fdb896ec49397aa1e) to disable the cron when the widget is disabled in Jetpack settings and if there are no active instances of the widget.

Currently it is not very optimal, as it relies on the shutdown hook, which may not be executed if Jetpack is completely disabled, but I have yet to find a better way to do it.

zinigor · 2016-01-12T21:21:09Z

Overall the changes look very nice, thanks for the awesome effort!

I can confirm that the existing widgets work fine after an upgrade, but when you add new ones, you have to wait a considerable amount of time. The problem right now is that if the user does not know the inner workings, to him/her this looks like an error. Should we perhaps:

trigger a synchronous update if there have been no previously stored options for that URL?
tell a user that they will see the error for some time? (That's probably not the best option)
hide the widget if there's nothing in cache? (That's probably even worse than the previous)

I'm happy to merge this as it is now on the beta stage, but only if we figure out a solution to the problem and implement it quickly enough before 3.9 hits.

bisko · 2016-01-14T20:33:14Z

@zinigor I improved the case of adding a new widget as per your suggestion.

It seems the most optimal way (transparent to the user) would be to force-fetch new data when saving new widget instance.

With the update, when a user adds a widget instance, it checks if there is a cache entry for the specified URL. If there isn't any entry in the cache, a forced fetch is done. This way the new widget data is shown immediately.

If an error occurs with the data fetch, it will be shown when the widget is saved, so the user will have immediate feedback if the remote fetch fails.

zinigor · 2016-01-15T14:45:47Z

Thank you for making changes! It works great! Now in order to clean up the branch, can you please fast-forward the master branch in your repo and rebase this branch so it wouldn't have any merge commits? Let me know if you need help with that.

…d and formatted

…or_storage()` Added tests for `format_posts_for_storage()`. Added error handling in format_posts_for_storage, because it was missing and it was relying on being supplied with proper data.

…dling in some methods. Added tests for `cron_task()`, `get_instances_sites()` and `update_instance()` methods. Improved error handling in `get_instances_sites()`. Removed error handling in `update_instance()` method, because it was not necessary there.

…s in `extract_errors_from_blog_data()` method. Wrong key, which was undefined in the context, was used in `reset()` call. It has been replaced with the proper key. Unit tests have been added to check for this case.

…log_data()` method with tests. Covered `extract_errors_from_blog_data()` with tests. Improved the error debug concatenation, to show a more readable message.

The widget was accessible by directly calling the PHP file. Check was added at the top of the file in order to prevent direct access.

…als() now respects other custom cron schedules. `jetpack_display_posts_widget_cron_intervals()` missed function parameter that was passed from `wp_get_schedules()`, containing other custom schedules. This caused all other schedules that were defined beforehand to be overwritten. Schedules, passed to the function are now properly handled and updated. Unit tests were added to make sure that it won't break everything again.

…on_update` action to be more clear that it is part of Jetpack. `display_posts_widget_cron_update` was missing the `jetpack_` prefix, which could have lead to some confusion that it was part of Jetpack. It has been properly prefixed now.

…h WordPress coding standards.

…outside widget constructor. `check_for_cron()` call was made inside the constructor of the widget. Since there can be multiple instances of the widget, the check would have been called multiple times, which is not very optimal. The check is now done when the `wp_loaded` is fired.

…pdate cron name to a static class property. The cron name will be used in several more places in the future (deactivation and checks) and it is better to be a variable, not hardcoded string.

…instead of directly accessing `body` property of service response. It is better to use `wp_remote_retrieve_body()` to extract request body and use it, instead of directly accessing the response property. This way we are safe of eventual future changes of service response format.

…better. Functions that related to fetching information for the remote blog were not ordered properly, which could have lead to confusion on how are they called and related to each other. Now they are grouped together and properly ordered.

…ation when there is no blog URL specified. There was no error when an empty URL field was left in the widget configuration. Now an error is shown if the URL field is empty.

…sabled if it is not needed. Previously if the widget was activated once, there was no way to stop the update cron and it would have been running in the background forever. Functionality has been added to allow cron disabling when it is not needed. A check is run on every shutdown to see if the cron should be running or not and the appropriate action is taken.

…ly disabled when Jetpack gets deactivated.

…ted and deactivated in more consistent way. Code was run on every shutdown of WordPress to check if the cron should have been running or not. This was a great overhead in cases where the widget wasn't needed to be shown. Now the cron is activated on the proper activation points - When Jetpack gets updated, when Extra Sidebar Widgets module gets activated and when Jetpack gets activated (if it was not activated before). Deactivation remains what it was before - when Extra Sidebar Widgets module gets deactivated and when Jetpack gets deactivated.

… widget save if it was not in cache. After saving the widget, widget data had to rely on the cron to be fetched. That could have lead to a user saving the widget and waiting for 10+ minutes, before seeing valid data on the sidebar and not an error. Now when the user saves widget configuration, if the specified site URL doesn't have cache entry, it is force-fetched and put in the cache, so the user can see the result immediately. Additionally remote requests are now cached in a static variable cache, to compensate for multiple requests that can happen when the widget is saved. It sheds a few seconds load time.

…o a mocked WP core method.

zinigor · 2016-01-18T11:55:13Z

Tested again after a rebase, works like a charm, thanks!

…vement Display Posts Widget: speed and stability improvements.

mdawaffe · 2016-01-19T03:08:07Z

modules/widgets/wordpress-post-widget.php

+ * If Jetpack is deactivated, the cron is not needed.
+ */
+add_action( 'jetpack_deactivate_module_widgets', 'Jetpack_Display_Posts_Widget::deactivate_cron_static' );
+register_deactivation_hook( 'jetpack/jetpack.php', 'Jetpack_Display_Posts_Widget::deactivate_cron_static' );


Jetpack may not be installed at jetpack/jetpack.php.

Fixed in #3303

samhotchkiss added the Test on WP.com label Dec 24, 2015

jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets [Status] Needs Review This PR is ready for review. labels Jan 4, 2016

jeherve added this to the 3.9 milestone Jan 4, 2016

jeherve added the Trial Project label Jan 4, 2016

mdawaffe reviewed Jan 5, 2016
View reviewed changes

mdawaffe reviewed Jan 6, 2016
View reviewed changes

zinigor added the [Status] Requires String Changes label Jan 12, 2016

zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jan 15, 2016

Change the stored data format and refactor the way the data is fetche…

66bdfbb

…d and formatted

bisko added 19 commits January 15, 2016 16:51

[Display Posts Widget] [unittests] Add unit tests for `format_posts_f…

4d3566c

…or_storage()` Added tests for `format_posts_for_storage()`. Added error handling in format_posts_for_storage, because it was missing and it was relying on being supplied with proper data.

[Display Posts Widget] [unittests] Fully cover `extract_errors_from_b…

5e46064

…log_data()` method with tests. Covered `extract_errors_from_blog_data()` with tests. Improved the error debug concatenation, to show a more readable message.

[Display Posts Widget] [fix] Disable direct access to widget code.

a7a3296

The widget was accessible by directly calling the PHP file. Check was added at the top of the file in order to prevent direct access.

[Display Posts Widget] [fix] Improve brace style to be consistent wit…

8cecb56

…h WordPress coding standards.

[Display Posts Widget] [improvement] Extracted Display Posts widget u…

9310499

…pdate cron name to a static class property. The cron name will be used in several more places in the future (deactivation and checks) and it is better to be a variable, not hardcoded string.

[Display Posts Widget] [improvement] Improve error message texts.

464fc19

[Display Posts Widget] [improvement] Show an error in widget configur…

47fdfe0

…ation when there is no blog URL specified. There was no error when an empty URL field was left in the widget configuration. Now an error is shown if the URL field is empty.

[Display Posts Widget] [improvement] Widget update cron is now proper…

78df508

…ly disabled when Jetpack gets deactivated.

[Display Posts Widget] [unittests] Add codeCoverageIgnore directive t…

fff5b6c

…o a mocked WP core method.

bisko force-pushed the update/display_posts_widget_improvement branch from fe7989c to fff5b6c Compare January 15, 2016 15:00

dereksmart mentioned this pull request Jan 18, 2016

Display Posts Widget shows Sharing buttons if "show excerpt" option is set #3290

Closed

dereksmart added a commit that referenced this pull request Jan 18, 2016

Merge pull request #3207 from bisko/update/display_posts_widget_impro…

5f5b4b5

…vement Display Posts Widget: speed and stability improvements.

dereksmart merged commit 5f5b4b5 into Automattic:master Jan 18, 2016

dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 18, 2016

mdawaffe reviewed Jan 19, 2016
View reviewed changes

kraftbj added Touches WP.com Files and removed Test on WP.com labels May 5, 2016

georgestephanis mentioned this pull request Mar 22, 2017

WPCOM Widgets Merge -- 22march2017 #6721

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display Posts Widget: speed and stability improvements. #3207

Display Posts Widget: speed and stability improvements. #3207

bisko commented Dec 24, 2015

mdawaffe Jan 5, 2016

bisko Jan 7, 2016

mdawaffe commented Jan 6, 2016

mdawaffe commented Jan 6, 2016

mdawaffe Jan 6, 2016

bisko Jan 7, 2016

mdawaffe commented Jan 6, 2016

mdawaffe commented Jan 6, 2016

mdawaffe Jan 6, 2016

bisko Jan 7, 2016

bisko commented Jan 7, 2016

bisko commented Jan 7, 2016

bisko commented Jan 7, 2016

zinigor commented Jan 12, 2016

bisko commented Jan 14, 2016

zinigor commented Jan 15, 2016

zinigor commented Jan 18, 2016

mdawaffe Jan 19, 2016

bisko Jan 19, 2016

Display Posts Widget: speed and stability improvements. #3207

Display Posts Widget: speed and stability improvements. #3207

Conversation

bisko commented Dec 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdawaffe commented Jan 6, 2016

mdawaffe commented Jan 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdawaffe commented Jan 6, 2016

mdawaffe commented Jan 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bisko commented Jan 7, 2016

bisko commented Jan 7, 2016

bisko commented Jan 7, 2016

zinigor commented Jan 12, 2016

bisko commented Jan 14, 2016

zinigor commented Jan 15, 2016

zinigor commented Jan 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment