Sync: Don't clear $this->just_published completely #6542

lezama · 2017-03-01T18:22:24Z

Fixes: https://github.com/Automattic/io/issues/19#issuecomment-281814105

Some plugins could hook into save_post and wp_insert_post from within the hook.

In that case we were clearing $this->just_published without waiting for the original post from being inserted.

How to test:
Publishing a post containing an embedded recipe using the WP Recipe Maker plugin replicates the scenario pointed above.
See: https://github.com/Automattic/io/issues/19#issuecomment-281814105

Some plugins could have a wp_insert_post hooked into the save_post action. In that case we were clearing $this->just_published without waiting for the original post from being inserted.

aduth · 2017-03-01T20:44:03Z

It appears there were a few build variations that failed, though it's unclear why they would and I'm unable to retrieve the logs. Perhaps just needs a build restart?

aduth · 2017-03-01T20:52:23Z

I'm not as familiar with when send_published is called and how just_published is assigned, but the logic of avoiding clearing all "just published" when sending a single post (in lieu if just clearing the one sent) sounds reasonable to me.

I've also run these changes in the context of my Jetpack site configured to Publicize through my sandbox and confirmed in the before/after case of the changes applied that both posts with and without a recipe Publicized successfully. 👍

support WP core < 4.7

…should_email_post_to_subscribers

… minute

jeherve · 2017-03-02T11:44:08Z

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

+			 * @param int post_id
+			 * @param mixed array post flags that are added to the post
+			 */
+


Looks like you have an extra empty line that will trip the parser.

jeherve · 2017-03-02T11:44:37Z

sync/class.jetpack-sync-sender.php

@@ -77,6 +77,7 @@ private function continue_full_sync_enqueue() {
 	}

 	public function do_sync() {
+		do_action( 'jetpack_sync_before_do_sync' );


Could you add a docblock here? Thanks!

always call `jetpack_sync_send_published` if the posts listener was listening :)

This reverts commit 109ec88.

lezama · 2017-03-03T01:31:58Z

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

 		add_action( 'deleted_post', $callable, 10 );
 		add_action( 'jetpack_publicize_post', $callable );
 		add_action( 'jetpack_published_post', $callable, 10, 2 );
 		add_action( 'transition_post_status', array( $this, 'save_published' ), 10, 3 );
 		add_filter( 'jetpack_sync_before_enqueue_wp_insert_post', array( $this, 'filter_blacklisted_post_types' ) );
-
+		


we need to remove this spaces, waiting for tests to pass first 😓

lezama · 2017-03-03T01:32:21Z

tests/php/modules/publicize/test_class.publicize.php

@@ -28,7 +28,7 @@ public function test_fires_jetpack_publicize_post_on_save_as_published() {
 		$this->post->post_status = 'publish';

 		wp_insert_post( $this->post->to_array() );
-
+		


more spaces to clean

The previous solution doesn’t fix the issue for core < 4.7

lezama · 2017-03-03T14:30:00Z

@aduth after 30 backs and forwards with @enejb :) I think it is good to go now. Could you give it another try?

aduth

I don't feel confidently speaking on other changes like those to subscriptions (though seems a little worrying to be removing logic to avoid resending notifications to an already-published post?), but as far as this affects Publicize sync: I've rerun with/without recipe on both 4.8-alpha and 4.6.3 to check varying logic and can confirm my posts publicize correctly in all cases.

aduth · 2017-03-03T14:50:16Z

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

@@ -190,24 +202,27 @@ function filter_post_content_and_add_links( $post_object ) {
 			 *
 			 * @since 4.5.0
 			 *
-			 * @param array of shortcode tags to remove.
+			 * @param array , of shortcode tags to remove.


Not clear why the comma was inserted here.

enejb · 2017-03-03T18:44:42Z

sync/class.jetpack-sync-sender.php

@@ -77,6 +77,12 @@ private function continue_full_sync_enqueue() {
 	}

 	public function do_sync() {
+		/**


This comment needs to go.

enejb · 2017-03-03T18:45:55Z

I lest a minor comment. I think once update the change this PR is ready to go.

Take care of

enejb

This is good to go!

@SInCE

* Readme: remove old release and add skeleton for 4.8. * Changelog: add #6572 * Changelog: add #6567 * Changelog: add #6542 * Changelog: add #6527 * Changelog: add #6508 * Changelog: add #6478 * Changelog: add #6477 * Changelog: add #6249 * Update stable version and remove old version from readme. * Changelog: add 4.7.1 to changelog. * Readme: add new contributor. * Sync: update docblock @SInCE version. Related: #6053 * Changelog: add release post. * changelog: add #6053 * Changelog: add #6413 * Changelog: add #6482 * Changelog: add #6584 * Changelog add #6603 * Changelog: add #6606 * Changelog: add #6611 * Changelog: add #6635 * Changelog: add #6639 * Changelog: add #6684 * Changelog: add #6710 * Changelog: add #6711 * Changelog: add #5461 * Testing list: update Settings UI feedback prompt. Props @MichaelArestad * Changelog: add #6789 * Changelog: add #6778 * Changelog: add #6777 * Changelog: add #6775 * Changelog: add #6755 * Changelog: add #6731 * Changelog: add #6721 * Changelog: add #6705 * Changelog: add #6702 * Changelog: add #6671 * Changelog: add #6637 * Changelog: add #6582 * Changelog: add #6566 * Changelog: add #6555 * Changelog: add #6529 * Changelog: add #6344 * Changelog: add #5763 * Changelog: add #5503 * Changelog: update #6637 changelog. @see 40e115c#commitcomment-21523982 * Changelog: add #6699 * Changelog: add #6632 * Changelog: add #6769 * Changelog: add #6707 * Changelog: add #6590

Don't clear $this->just_published completely

6a0cb6a

Some plugins could have a wp_insert_post hooked into the save_post action. In that case we were clearing $this->just_published without waiting for the original post from being inserted.

lezama added [Package] Sync [Team] Poseidon [Type] Bug When a feature is broken and / or not performing as intended labels Mar 1, 2017

lezama self-assigned this Mar 1, 2017

lezama requested review from enejb, ebinnion and aduth March 1, 2017 18:22

jeherve added the [Status] Needs Review This PR is ready for review. label Mar 1, 2017

do send_published on shutdown

bcfb1e8

support WP core < 4.7

enejb mentioned this pull request Mar 2, 2017

Set publish_post event on shutdown. #6546

Closed

lezama added 4 commits March 1, 2017 23:15

Subscriptions: remove unnecessary checks as last decision depends on …

b8dea5c

…should_email_post_to_subscribers

call do_sync(); so jetpack_published_call gets hooked at the last…

01f122d

… minute

fix test logic

bb2bd84

fix publicize test

c0da915

jeherve previously requested changes Mar 2, 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 Mar 2, 2017

lezama and others added 10 commits March 2, 2017 12:32

do_sync isn't always invoked

7fd8061

always call `jetpack_sync_send_published` if the posts listener was listening :)

fix phpcs warnings

497ba3e

remove empty line

1aab5b8

remove no longer necessary test

6c3c2e2

re add jetpack_sync_before_do_sync

8fd2ca4

fix tests

7a178b7

Make sure that post is an object before trying to publish it.

109ec88

Revert "Make sure that post is an object before trying to publish it."

8caf9c7

This reverts commit 109ec88.

reset just_published before each test

1898952

add 'shutdown_test' action

5a99893

remove no longer needed logic

e53f062

lezama commented Mar 3, 2017

View reviewed changes

lezama added 7 commits March 2, 2017 22:37

fix test

762edf8

aweful hack

7f2e955

rewind things a bit

0352774

The previous solution doesn’t fix the issue for core < 4.7

Improve the test and make it pass for old WP versions

80a550e

reformat code

7897d5b

remove duplicated check

622bb0d

undo

e039f95

aduth reviewed Mar 3, 2017

View reviewed changes

lezama added 2 commits March 3, 2017 14:25

use the same custom wp_insert_post method for every core version

c38a132

fix phpdocs

c1a00e6

enejb reviewed Mar 3, 2017

View reviewed changes

remove comment

7ee951f

enejb approved these changes Mar 3, 2017

View reviewed changes

lezama merged commit 6a83fb5 into master Mar 3, 2017

lezama deleted the update/jetpack_published_post_logic branch March 3, 2017 22:09

matticbot removed the [Status] Needs Review This PR is ready for review. label Mar 3, 2017

jeherve added [Status] Needs Changelog and removed [Status] Needs Changelog labels Mar 6, 2017

jeherve added a commit that referenced this pull request Mar 9, 2017

Changelog: add #6542

17a4b52

jeherve added a commit that referenced this pull request Mar 28, 2017

Changelog: add #6542

d20c22d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync: Don't clear $this->just_published completely #6542

Sync: Don't clear $this->just_published completely #6542

lezama commented Mar 1, 2017 •

edited

Loading

aduth commented Mar 1, 2017

aduth commented Mar 1, 2017

jeherve Mar 2, 2017

jeherve Mar 2, 2017

lezama Mar 3, 2017

lezama Mar 3, 2017

lezama commented Mar 3, 2017

aduth left a comment

aduth Mar 3, 2017

enejb Mar 3, 2017

enejb commented Mar 3, 2017

enejb left a comment

		@@ -28,7 +28,7 @@ public function test_fires_jetpack_publicize_post_on_save_as_published() {
		$this->post->post_status = 'publish';

		wp_insert_post( $this->post->to_array() );

Sync: Don't clear $this->just_published completely #6542

Sync: Don't clear $this->just_published completely #6542

Conversation

lezama commented Mar 1, 2017 • edited Loading

aduth commented Mar 1, 2017

aduth commented Mar 1, 2017

jeherve Mar 2, 2017

Choose a reason for hiding this comment

jeherve Mar 2, 2017

Choose a reason for hiding this comment

lezama Mar 3, 2017

Choose a reason for hiding this comment

lezama Mar 3, 2017

Choose a reason for hiding this comment

lezama commented Mar 3, 2017

aduth left a comment

Choose a reason for hiding this comment

aduth Mar 3, 2017

Choose a reason for hiding this comment

enejb Mar 3, 2017

Choose a reason for hiding this comment

enejb commented Mar 3, 2017

enejb left a comment

Choose a reason for hiding this comment

lezama commented Mar 1, 2017 •

edited

Loading