-
Notifications
You must be signed in to change notification settings - Fork 808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sync: Add links when expanding posts at a later stage #37926
Sync: Add links when expanding posts at a later stage #37926
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
I know this is still in progress however I left some related thoughts in D152661-code. Mostly concerns around removing a public method instead of deprecating and the performance cost of running |
…reaking change and we can still use it for expand_posts and get_object_by_id
…to make sure we are behaving in the same way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing with WPML and consistently getting wrong permalinks unfortunately. Not sure if this is worth pursuing if we are not able to get the right permalinks with the WPML plugin since it's one of the main reasons we are making this change..
Does the fix in https://github.com/Automattic/jpop-issues/issues/8721 get the proper permalinks? |
It doesn't for me, I tested that too.. I was testing with the latest WPML version on trunk and adding the suggested filters via Code Snippets |
Closing this since it doesn't make sense to go with this approach if the JPOP issue https://github.com/Automattic/jpop-issues/issues/8721 does not get solved |
Fixes https://github.com/Automattic/vulcan/issues/297
Proposed changes:
When syncing posts, we used to expand them via the
filter_post_content_and_add_links
. This was originally done before sending, but after dedicated sync was created, we decided to move it to before enqueuing the actions.Since sometimes the links like permalink are not available. The approach here is to separate the filtering part from the adding links.
The expectation is that adding the links before we send the action, as we used to do will allow us to fetch the proper links.
The possible problem here is that with Dedicated Sync enabled, we would be fetching those fields on init because this is when we spawn the next dedicated sync request.
Other information:
Does this pull request change what data or activity we track or use?
No
Testing instructions:
The initial idea here is to test that we are still properly populating the links.
In a connected Jetpack site with this patch.
jetpack_sync_save_post
The second one, when the post is published, has the proper permalink.Now test with a plugin that will change the permalink like Yoast SEO.
jetpack_sync_save_post
jetpack_sync_save_post
has changed.Bonus points if tested with WPML plugin as described in the links inside the issue