Skip to content

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

Merged
lezama merged 35 commits intomasterfrom
update/jetpack_published_post_logic
Mar 3, 2017
Merged

Sync: Don't clear $this->just_published completely#6542
lezama merged 35 commits intomasterfrom
update/jetpack_published_post_logic

Conversation

@lezama
Copy link
Copy Markdown
Contributor

@lezama lezama commented Mar 1, 2017

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.
@lezama lezama added [Package] Sync [Team] Poseidon Bug When a feature is broken and / or not performing as intended labels Mar 1, 2017
@lezama lezama self-assigned this Mar 1, 2017
@lezama lezama requested review from aduth, ebinnion and enejb March 1, 2017 18:22
@aduth
Copy link
Copy Markdown
Member

aduth commented Mar 1, 2017

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
Copy link
Copy Markdown
Member

aduth commented Mar 1, 2017

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. 👍

@jeherve jeherve added the [Status] Needs Review This PR is ready for review. label Mar 1, 2017
support WP core < 4.7
jeherve
jeherve previously requested changes Mar 2, 2017
* @param int post_id
* @param mixed array post flags that are added to the post
*/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

public function do_sync() {
do_action( 'jetpack_sync_before_do_sync' );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a docblock here? Thanks!

@jeherve 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
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' ) );

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more spaces to clean

@lezama
Copy link
Copy Markdown
Contributor Author

lezama commented Mar 3, 2017

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

Copy link
Copy Markdown
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @since 4.5.0
*
* @param array of shortcode tags to remove.
* @param array , of shortcode tags to remove.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear why the comma was inserted here.

}

public function do_sync() {
/**
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs to go.

@enejb
Copy link
Copy Markdown
Member

enejb commented Mar 3, 2017

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

@enejb enejb dismissed jeherve’s stale review March 3, 2017 19:25

Take care of

Copy link
Copy Markdown
Member

@enejb enejb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good to go!

@lezama lezama merged commit 6a83fb5 into master Mar 3, 2017
@lezama lezama deleted the update/jetpack_published_post_logic branch March 3, 2017 22:09
@matticbot matticbot removed the [Status] Needs Review This PR is ready for review. label Mar 3, 2017
jeherve added a commit that referenced this pull request Mar 9, 2017
jeherve added a commit that referenced this pull request Mar 28, 2017
samhotchkiss pushed a commit that referenced this pull request Mar 29, 2017
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug When a feature is broken and / or not performing as intended [Package] Sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants