add support for publishpress revisions plugin (WP-934) - #634
Conversation
…s published (WP-934) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ed (WP-934) Ignore post_status and post_name in the content hash, accept legacy hash for existing submissions, and suppress change detection while PublishPress Revisions applies a revision. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…gument checks (WP-934) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
sl-mmuradov
left a comment
There was a problem hiding this comment.
Reviewed against WP-934 and the PublishPress Revisions source (admin/revision-action_rvy.php). The hook names and argument order are correct. The inline comments cover the main issues: one upgrade/migration concern, one ordering bug, one gap against the ticket's goal (locales going live together), a stale hash, and two tests that don't exercise their code paths.
| 'post_password', | ||
| 'post_modified', | ||
| 'post_modified_gmt', | ||
| 'post_status', |
There was a problem hiding this comment.
Upgrade impact: every existing submission becomes outdated. Every source_content_hash already stored was calculated with post_status and post_name included. Commit 4449066 removed the legacy-hash fallback, so after upgrade the next save_post on any translated post (even a no-op save) fails the hash comparison in DetectChangesHelper::update(). That marks the submissions Outdated. On profiles with UPLOAD_ON_CHANGE_AUTO, it also sets them to New and queues a re-upload, so this would trigger a mass re-upload across all installs.
Please bring back the fallback (also accept the hash calculated the old way, then store the new hash), or add a migration that recalculates stored hashes. This should also get a readme/changelog entry.
Also, excluding post_name globally means a slug change on any post no longer marks its translations outdated, not just for revisions. Is that intended?
There was a problem hiding this comment.
Yes, that's a tradeoff I took. Saving a post is usually a manual operation. Post slugs change is intended as well.
| return; | ||
| } | ||
|
|
||
| $this->wpProxy->add_filter('revisionary_apply_revision_data', [$this, 'moveSubmissionsToOriginal'], 10, 3); |
There was a problem hiding this comment.
Submissions are moved before the revision is actually applied. revisionary_apply_revision_data fires at revision-action_rvy.php:935, before rvy_update_post() / wp_update_post() at :962. If that update fails, PP returns early at :969, which leaves things in this state:
- the revision is neither applied nor deleted, but its submissions already point at the original;
- the original's existing submission has already been deleted in
moveSubmission(); revision_appliednever fires, soDetectChangesHelperstays suppressed for the original for the rest of the request.
Only commit the move once the update has succeeded. One option: record the move as pending here, then perform it from before_delete_post for that revision ID, which only runs after a successful update (:1155). That handler could run with a priority ahead of SubmissionCleanupHelper, or the cleanup could skip revision IDs that are marked as being applied.
| ); | ||
|
|
||
| if ($existing !== null) { | ||
| if ($existing->getTargetId() > $submission->getTargetId()) { |
There was a problem hiding this comment.
The old translation stays live and orphaned; the new translation stays a draft. In the main scenario from the ticket (a ToS page that is already translated), $existing is unlinked and deleted, but its target post stays published in every target blog and is no longer linked to anything. The linked translation becomes the revision's target, which is a draft. Publishing that draft will likely get a -2 permalink, because the old post still holds the slug. So after the source is published, the locales still serve the old content, which doesn't meet the ticket's goal of all locales going live at the same time.
Suggestion: keep the existing target post (same ID, slug and status) and apply the revision target's content to it, or re-download into it. Then drop the draft target. If that's out of scope, the manual steps need to be documented.
Separately, comparing getTargetId() values to decide which translation is newer is fragile, because post IDs aren't timestamps. Comparing the submissions' applied_date / last_modified would be more reliable.
There was a problem hiding this comment.
Added a readme entry regarding translation status. Comparing applied_date now.
| return $update; | ||
| } | ||
|
|
||
| public function resumeChangeDetection(mixed $originalId): void |
There was a problem hiding this comment.
The moved submissions keep the hash calculated from the revision post. That hash includes revision-only meta such as _rvy_base_post_id, and _requested_slug where present, which the original doesn't have. Suppression only covers the apply request, so the next unrelated save of the original marks every moved submission Outdated, and re-uploads them with auto-upload.
Suggestion: here, recalculate and store source_content_hash for the moved submissions that were up to date before the move. Alternatively, add _rvy_* meta to ContentSerializationHelper::getRemoveFields()['meta'].
There was a problem hiding this comment.
Fixed by checking while the revision is intact
| $this->x($manager, null, $wp)->moveSubmissionsToOriginal([], (object)['ID' => 20], (object)['ID' => 10, 'post_type' => 'page']); | ||
| } | ||
|
|
||
| public function testFailureDoesNotBreakRevisionPublishing(): void |
There was a problem hiding this comment.
This test doesn't reach the code it's named for. The revision and published objects have no post_type, so moveSubmissionsToOriginal() returns early from the argument check at PublishPressRevisions.php:79-87. find() is never called, so the \Throwable catch isn't exercised. The same applies to testExistingOriginalSubmissionWithNewerTargetIsKept (line 81): the revision object lacks post_type, so its never() expectations pass trivially and the newer-target branch is untested. Add 'post_type' => 'page' to both objects, and assert that find is actually invoked.
Move submissions only after the revision was applied, recalculate hash of moved submissions, compare applied dates, fix tests, add readme entry. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
| - [ "addService", [ "@smartling.helper.relative-image-path-support" ]] | ||
| - [ "addService", [ "@smartling.helper.absolute-image-path-support" ]] | ||
| - [ "addService", [ "@service.submission-cleanup" ]] | ||
| - [ "addService", [ "@plugin.publishpress-revisions" ]] |
There was a problem hiding this comment.
Just an idea, can we register these services dynamically? Like this service only makes sense to be loaded if current WP instance really has publishpress plugin installed, seems like.
No action points, just an idea.
There was a problem hiding this comment.
It checks in the register handler if the plugin is active and of supported version, and if not, registers no hooks
| { | ||
| use LoggerSafeTrait; | ||
|
|
||
| public const REVISION_STATUSES = ['draft-revision', 'pending-revision', 'future-revision']; |
There was a problem hiding this comment.
How exactly connector works with these revisions? Is there a way to setup content from which state we grab and send to smartling? Like, draft or published.
There was a problem hiding this comment.
The way the revisions plugin works, it is possible to pick any revision and send content for translation. The download will, however, apply to the last modified version on the target site
No description provided.