Sync: Add sync actions when core updates by enejb · Pull Request #7108 · Automattic/jetpack

enejb · 2017-05-02T19:50:35Z

The new action lets us know when core gets updated to a specific
version and it was autoupdated and if it was reinstalled.

Changes proposed in this Pull Request:

Send specific events that would allow us know when a site has autoupdated, updated or reinstalled core.

Testing instructions:

Go to the wp-admin/update-core.php and click on the update core button.
.com should get the jetpack_sync_update_core_successfull action with 4 arguments.
Update the core verions to help trigger the core update links.
Do the same. And notice that we get jetpack_sync_update_core_successfull again but with different arguments.

Let autoupdate update core and notice the same action.

To the phpunit tests pass?

Proposed changelog entry for your changes:

The new action lets us know when core gets updated to a specific version and it was autoupdated and if it was reinstalled.

gititon · 2017-05-03T18:50:11Z

sync/class.jetpack-sync-module-updates.php


 	const UPDATES_CHECKSUM_OPTION_NAME = 'jetpack_updates_sync_checksum';

+	private $old_version = null;


It looks like there is functionality related to the wordpress updates, plugin updates, theme updates, etc. going on in this file. For that reason, it may be worth renaming this property to $old_wp_version to clarify how it is used, though I wouldn't consider this a blocking change.

gititon · 2017-05-03T18:51:08Z

sync/class.jetpack-sync-module-updates.php

+		add_action( '_core_updated_successfully', array( $this, 'update_core' ) );
+		add_action( 'jetpack_sync_update_core_successfull', $callable, 10, 2 );
+		add_action( 'jetpack_sync_autoupdate_core_successfull', $callable, 10, 2 );
+		add_action( 'jetpack_sync_reinstall_core_successfull', $callable, 10, 1 );


When you have priority 10 and a single argument, can't you omit these parameters? This isn't a blocking change, of course.

gititon · 2017-05-03T18:51:36Z

sync/class.jetpack-sync-module-updates.php

+	public function update_core( $new_version ) {
+		global $pagenow;
+
+		if ( isset( $_GET[ 'action' ] ) && 'do-core-reinstall' == $_GET[ 'action' ] ) {


Do you want to use === instead of == ?

gititon · 2017-05-03T18:52:27Z

sync/class.jetpack-sync-module-updates.php

+		global $pagenow;
+
+		if ( isset( $_GET[ 'action' ] ) && 'do-core-reinstall' == $_GET[ 'action' ] ) {
+			do_action( 'jetpack_sync_reinstall_core_successfull', $new_version );


successful has 1 l

gititon · 2017-05-03T18:52:35Z

sync/class.jetpack-sync-module-updates.php

+
+		// Core was autoudpated
+		if ( 'update-core.php' !== $pagenow ) {
+			do_action( 'jetpack_sync_autoupdate_core_successfull', $new_version, $this->old_version );


gititon · 2017-05-03T18:52:43Z

sync/class.jetpack-sync-module-updates.php

+			return;
+		}
+
+		do_action( 'jetpack_sync_update_core_successfull', $new_version, $this->old_version );


lezama · 2017-05-04T16:05:23Z

sync/class.jetpack-sync-module-updates.php

+
+		// Send data when update completes
+		add_action( '_core_updated_successfully', array( $this, 'update_core' ) );
+		add_action( 'jetpack_sync_update_core_successful', $callable, 10, 2 );


I think we could call this one jetpack_core_updated_successfully to match the core one.

lezama · 2017-05-04T16:08:05Z

tests/php/sync/test_class.jetpack-sync-updates.php


 	public function test_sync_wp_version() {
 		global $wp_version;
+		$previews_version = $wp_version;


previous :)

jeherve

A small request to make our parser happy :)

jeherve · 2017-05-04T16:58:54Z

sync/class.jetpack-sync-module-updates.php

+		global $pagenow;
+
+		if ( isset( $_GET[ 'action' ] ) && 'do-core-reinstall' == $_GET[ 'action' ] ) {
+			do_action( 'jetpack_sync_reinstall_core_successful', $new_wp_version );


Don't forget the docblocks :)

jeherve · 2017-05-04T16:58:58Z

sync/class.jetpack-sync-module-updates.php

+
+		// Core was autoudpated
+		if ( 'update-core.php' !== $pagenow ) {
+			do_action( 'jetpack_sync_autoupdate_core_successful', $new_wp_version, $this->old_wp_version );


Don't forget the docblocks :)

jeherve · 2017-05-04T16:59:02Z

sync/class.jetpack-sync-module-updates.php

+			return;
+		}
+
+		do_action( 'jetpack_sync_update_core_successful', $new_wp_version, $this->old_wp_version );


Don't forget the docblocks :)

enejb · 2017-05-04T19:06:53Z

@jeherve can you take another look?

jeherve · 2017-05-04T20:22:03Z

@enejb I think you'll need to push your changes first.

enejb · 2017-05-05T16:10:27Z

Ayayay changes pushed

lezama · 2017-05-08T13:33:13Z

sync/class.jetpack-sync-module-updates.php

+
+		if ( isset( $_GET[ 'action' ] ) && 'do-core-reinstall' == $_GET[ 'action' ] ) {
+			/**
+			 * Sync event that is fires when the core reinstall was successful


We should rephrase:

Sync event that is fires when the core reinstall was successful

"Fired on successful core reinstall"? @gititon could you help us here? :)

lezama · 2017-05-08T13:33:32Z

sync/class.jetpack-sync-module-updates.php

+			return;
+		}
+		/**
+		 * Sync event that is fires when the core update was successful


need rephrasing too

lezama

I think it is ready after handling the === comment

fixed the issue reported

@see

* Changelog: first pass at a changelog for 5.0 * Changelog: delete 4.9 testing list. * Changelog: update minimum WP version to match ver. in jetpack.php Fixes #7158 * Changelog: add #6051 * Changelog: add #6753 * Changelog: add #6928 * Changelog: add #6964 * Changelog: add #7014 * Changelog: add #7057 * Changelog: add #7060 * Changelog: add #7068 * Changelog: add #7070 * Changelog: add #7072 * Changelog: add #7071 * Changelog: add release date and post shortlink. * Changelog: add #7094 * Changelog: add #7100 * Changelog: add #7108 * Changelog: add #7113 * Changelog: add #7123 * Changelog: add #7135 * Changelog: add #7143 * Changelog: add #7151 * Changelog: add #6996 * Changelog: add #7105 * Changelog: add #7132 * Changelog: add #7166 * Changelog: fix typo in 4.9 changelog. * Changelog: remove older releases' changelogs. @see p1HpG7-42e-p2 * Changelog: add #7090 * Changelog: add #7095 * Changelog: add #7112 * Changelog: add #7115 * Changelog: add #7122 * Changelog: add #7137 * Changelog: add #7138 * Changelog: add #7140 * Changelog: add #7154 * Changelog: add ##7155 * Changelog: add #7163 * Changelog: add #7167 * Changelog: add #7171 * Changelog: add #7180 * Changelog: add #7181 * Changelog: add #7183 * Changelog: add #7184 * Changelog: add #7189 * Changelog: add #7191 * Changelog: add #7193 * Changelog: add #7198 * Changelog: add #7200 * Changelog: add #7209 * Changelog: add #7212 * Testing list: add instructions for #7115 * Changelog: add #7188 * Changelog: add #7205 * Changelog: add #7225 * Changelog: add #6872 * Changelog: add #7107 * Changelog: add #7118 * Changelog: add #7142 * Changelog: add #7170 * Changelog: add #7210 * Changelog: add #7218 * Changelog: add #7232 * Changelog: add #7211 * Changelog: add #7213 * Changelog: add #7229 * Changelog: add #7230 * Changelog: add #7214 * Draft changelog for 5.0 * Changelog updates: 2nd pass at a clearer changelog. - Fix typos. - Use consistent tense and tone across all changelog. - Remove unclear items. * Changelog: add #7026 * Changelog: add #7058 * Changelog: add #7125 * Changelog: add #7249 * Changelog: add #7185 * add mentions of image widget migration * Changelog: add info about new output for CLI command. * Changelog: add WP version number matching the new Image Widget.

Sync: Add sync actions when core updates

673228a

The new action lets us know when core gets updated to a specific version and it was autoupdated and if it was reinstalled.

enejb added [Package] Sync [Status] Needs Review This PR is ready for review. [Status] Needs Testing We need to add this change to the testing call for this month's release [Team] Poseidon labels May 2, 2017

enejb added 2 commits May 2, 2017 13:10

Split the events into 3 different events.

2ac5d28

Remove var_dump

83f4872

enejb requested review from gititon and lezama May 3, 2017 16:48

gititon reviewed May 3, 2017

View reviewed changes

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels May 3, 2017

enejb added 2 commits May 3, 2017 18:32

Minor fixes, spelling and more clear variable names

30e2e8b

Fix typo, make the tests pass

2f42fce

gititon approved these changes May 4, 2017

View reviewed changes

lezama reviewed May 4, 2017

View reviewed changes

jeherve previously requested changes May 4, 2017

View reviewed changes

enejb added [Status] Needs Review This PR is ready for review. 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 May 4, 2017

Update with fixes, added doc, fixed names

d1180d2

lezama reviewed May 8, 2017

View reviewed changes

Minor grammer fix

e3410c3

lezama removed the [Status] Needs Review This PR is ready for review. label May 8, 2017

lezama approved these changes May 8, 2017

View reviewed changes

minor now we user ===

594ad08

enejb merged commit a27d514 into master May 9, 2017

enejb deleted the add/sync-core-update branch May 9, 2017 20:25

jeherve added [Status] Needs Changelog and removed [Status] Needs Changelog labels May 9, 2017

jeherve added a commit that referenced this pull request May 11, 2017

Changelog: add #7108

4d0aa14

jeherve removed the [Status] Needs Testing We need to add this change to the testing call for this month's release label May 23, 2017

jeherve added a commit that referenced this pull request May 29, 2017

Changelog: add #7108

725f02d


		const UPDATES_CHECKSUM_OPTION_NAME = 'jetpack_updates_sync_checksum';

		private $old_version = null;

Conversation

enejb commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

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!

Choose a reason for hiding this comment

Uh oh!

jeherve 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enejb commented May 4, 2017

Uh oh!

jeherve commented May 4, 2017

Uh oh!

enejb commented May 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lezama left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

enejb commented May 2, 2017 •

edited

Loading