Jetpack: Subscriptions: filter to exclude posts by category results in emails being sent anyway

[tiffanybridge]

Steps to reproduce the issue

1. Use test code: add_filter( 'jetpack_subscriptions_exclude_these_categories', function( $categories ) { $categories = array( 'exclude' ); return $categories; });
2. modify the $categories array to contain only one category to exclude called exclude.
3. Create a post using the block editor and assign category exclude, then publish without first saving as Draft
4. The emails go out via Jetpack subscriptions anyway!

What I expected

To be able to exclude emails from Jetpack Subscriptions using a specified category.

What happened instead

Jetpack sends the email anyway.

Our testing on a partner site indicates that this is related to the way the WordPress/the block editor sets the post meta after the post is saved; Jetpack doesn't have the information needed to decide to exclude the post .

Seems related to this: https://github.com/Automattic/jetpack/issues/11445

[kraftbj]

Marking it as high because:

1. It looks to be a regression with moving to Gutenberg and thus publishing a post via the wp-json API.
2. Subscriptions have always been a little brittle in terms of something "weird" with publishing... scheduled posts, for example. I have a feeling that the underlying cause may all be related, so want to ensure we investigate this sooner rather than later. Going with my gut.

[kraftbj]

Tracking this down to for a REST API post submission, the sync does fire as expected, including the just_published action in the posts module ( https://github.com/Automattic/jetpack/blob/master/packages/sync/src/modules/class-posts.php#L559 ).

But, that function is firing hooks attached to jetpack_published_post_flags. When using the classic editor, the functions attached to it (should_email_post_to_subscribers) fires, but via the REST API nope. Coming back to this tomorrow.

[kraftbj]

I spent way too long and tracked it down to, what I consider, a Core bug: https://core.trac.wordpress.org/ticket/45114

Basically, the REST API fires off the functions that fire the wp_insert_post hook before the categories have been set to a post. Since Core hasn't set categories, the logic to exclude a post won't match, and the post is e-mailed out.

I suspect this is responsible for far more than just this bug. :(

[kraftbj]

@mdbitz I uploaded a POC on a branch to address this particular issue. There are more cases (attachments, page, CPTs, etc), but do you think this is viable? Basically, not firing our sync actions for REST API insert requests and waiting for the later REST filter?

[jeherve]

Noting #11752 as a potential related issue.

[kraftbj]

This would impact anything expecting categories, terms, or post meta to be on the post at the wp_insert_post or other related hooks.

[mdbitz]

Was reviewing this and took me a little to get my head around the explicit passing of null for the post object. but that is needed to get around the post_type check.

My main concern that I need to check is if this is going to cause the set_object_terms to fail on WP.com for new posts.

E.g.

Call to REST API to insert post

• we defer the insert sync action
• however set_object_terms and others are going to be triggered for the post_id
• these actions are going to fail on the cache site since the post doesn't exist until the later shim

[mdbitz]

Testing this and with the POC I'm not seeing the final jetpack_sync_save_post sync action come through.

My concern around not having an existing post_id is not a concern when in the gutenberg editor as it pre-creates a post with "new" status. So this approach will work if we get the action to trigger properly in the shim.

However outside of Gutenberg editor this would cause issues when the rest-api is used directly to

[kraftbj]

That makes sense @mdbitz. Thanks for the look. I could do a similar approach as with my Core patch in https://core.trac.wordpress.org/ticket/45114.

If the request is from the REST API and creating a new post, we save the intended post status, replace it with "new", let the post sync and add a hook on to the end of the REST API process to then have us fire a sync again with the current post status... I'll pick at this a bit more; thanks again.

[NickGreen]

@kraftbj Hey there! I'm following up with this issue. It seems like it's been decided to handle this in WP core, which is probably the most responsible way to go, since it has potential to solve a lot of other issues as well. It seems like that core issue stalled out, however.

We are looking into a patch for the site that started this issue, but would still like to advocate for continuing with the core fix for this.

[kraftbj]

@mdbitz What would you think about something like core.trac.wordpress.org/ticket/45114#comment:28 for new post saves?

[jeherve]

Noting that https://core.trac.wordpress.org/changeset/49172 should help us a bit, at least for our users who use WordPress 5.6. Maybe we can update Sync accordingly? cc @mdbitz

[mdbitz]

Actively looking into migrating to wp_after_insert_post for the jetpack_published_post event