Fix lock screen not saving changes (WP-1019) - #632
vsolovei-smartling wants to merge 3 commits into
Conversation
…ulk-action nonce (WP-1019) TranslationLockTableWidget::display() (inherited from WP_List_Table) renders its own hidden `_wpnonce` field for its unused bulk actions. Our own CSRF nonce field used the same name, so the popup form submitted two identically named `_wpnonce` inputs; PHP kept only the last one in $_POST, which never matched the 'smartling-translation-lock-action' nonce, so every save was silently rejected. Renamed our field to `smartling_lock_nonce` to avoid the collision. Verified live against a real WordPress install that this reproduces the reported symptom before the fix and resolves it after. Also removes a stray duplicate `@wp.proxy` argument in wp.translation.lock's services.yml definition left over from the WP-1015 nonce refactor (harmless - PHP ignores extra positional constructor arguments - but confusing dead config). Adds a PHPUnit regression test guarding the field name, and a Playwright E2E test (with a seeding fixture) that opens the real popup, unlocks fields, and asserts the change persists after Save. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… welcome-guide overlay (WP-1019) Running the E2E test live surfaced two issues invisible from static review: - create-locked-submission.php keyed its idempotency check on (source_blog_id, source_id, target_blog_id, target_id, content_type). Re-running it with a different source post (as happens across repeated local/CI runs) left two submission rows targeting the same post. TranslationLockController::getSubmission() -> findOne() returns null when more than one row matches (ambiguous-match guard), which silently hid the Translation Lock meta box entirely - reproduced live via a real browser session and confirmed with debug logging. Re-keyed the lookup to (target_blog_id, target_id, content_type) - the same uniqueness getSubmission() assumes - and made the fixture defensively collapse any pre-existing duplicates to one row. - translation-lock.spec.js didn't account for the one-time-per-user "Welcome to the editor" Gutenberg guide, which overlays the whole screen on a fresh user's first visit (the E2E DB is fresh every run). Dismiss it with Escape rather than clicking its Close button - the click is flaky while the modal is still animating in, since the modal's own overlay briefly intercepts pointer events aimed at its own content. Verified via a real Chromium run against a completely fresh test user: fails on both this hazard and the fixture ambiguity if unfixed, passes 100% of the time once fixed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Patch bump: Translation Lock nonce-collision fix and its test coverage introduce no new features or breaking changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PavelLoparev
left a comment
PavelLoparev
left a comment
There was a problem hiding this comment.
Automated review (claude-pr-review-local). Core PHP fix looks correct and well-targeted at the described nonce-collision root cause. Inline notes below on a few items; one architectural concern that doesn't map to a line in this diff:
Recommendation: same bug pattern likely still live elsewhere
The root cause here is: a controller renders its own wp_nonce_field(..., '_wpnonce') and then calls ->display() on a WP_List_Table subclass, which unconditionally renders its own hidden _wpnonce field in display_tablenav('top') - two same-named inputs collide, and PHP keeps only the list table's value in $_POST, silently failing verification.
That exact pattern also appears, unfixed, in:
BulkSubmitTableWidget(BULK_ACTION_NONCE_FIELD = '_wpnonce') rendered viaBulkSubmit.phpSubmissionTableWidget(BULK_ACTION_NONCE_FIELD = '_wpnonce', and it does defineget_bulk_actions(), so the tablenav nonce field is definitely rendered) rendered viaSubmissionsPage.php
Both verify against BULK_ACTION_NONCE_FIELD/BULK_ACTION_NONCE_ACTION the same way TranslationLockController did before this fix. If the analysis in this PR is correct, Bulk Submit form submission and the Submissions-page bulk actions are likely also currently silently rejecting valid nonce checks in production. Recommend filing a follow-up ticket and auditing every WP_List_Table subclass for this pattern before treating WP-1019 as fully resolved.
Ready to merge? With fixes
Reasoning: The core fix is correct and tested, but the same bug class appears to remain live in two sibling widgets, and the new E2E fixture has a silent-skip failure mode that undercuts the regression guarantee the ticket's DoD asks for.
| --url="${E2E_DOMAIN}") | ||
| echo "${LOCK_FIXTURE_OUTPUT}" | ||
| E2E_LOCK_TARGET_BLOG_PATH=$(echo "${LOCK_FIXTURE_OUTPUT}" | grep -oP 'E2E_LOCK_TARGET_BLOG_PATH=\K\S+') | ||
| E2E_LOCK_TARGET_POST_ID=$(echo "${LOCK_FIXTURE_OUTPUT}" | grep -oP 'E2E_LOCK_TARGET_POST_ID=\K\d+') |
There was a problem hiding this comment.
🟡 Warning: No exit-code/empty check after extracting E2E_LOCK_TARGET_BLOG_PATH/E2E_LOCK_TARGET_POST_ID via grep -oP. If create-locked-submission.php fails (non-zero exit) or the WP-CLI output format changes so these patterns stop matching, both vars end up empty and translation-lock.spec.js will just test.skip() - a regression test written specifically to catch "silently failing to save" can itself silently stop running in CI with no red build.
Suggested fix:
[ -n "$E2E_LOCK_TARGET_POST_ID" ] || { echo "Failed to extract E2E_LOCK_TARGET_POST_ID"; exit 1; }| } | ||
|
|
||
| if ($existingSubmissionId) { | ||
| $wpdb->update( |
There was a problem hiding this comment.
🟡 Warning: The return value of $wpdb->update() isn't checked here, unlike the insert branch below (which checks $wpdb->insert_id and calls WP_CLI::error()). If the update silently fails (schema drift, lock wait timeout, etc.) this fixture reports success and the Playwright test later fails on an unrelated assertion, obscuring the real cause.
$result = $wpdb->update(...);
if (false === $result) {
WP_CLI::error('Failed updating locked submission: ' . $wpdb->last_error);
}| * verifyLockActionNonce() would reject every save with a valid-looking but | ||
| * wrong nonce. See TranslationLock.php and TranslationLockTableWidget.php. | ||
| */ | ||
| public function testNonceFieldNameDoesNotCollideWithListTableBulkNonce(): void |
There was a problem hiding this comment.
🔵 Suggestion: This test only asserts LOCK_ACTION_NONCE_FIELD !== '_wpnonce' - it doesn't exercise any real rendering/collision behavior, so it would pass trivially for any other name, including one that collides for a different reason. The real regression coverage lives entirely in the new Playwright spec; consider either dropping this unit test or strengthening it to assert against TranslationLockTableWidget's actual rendered nonce field name rather than a literal string.
| - "@content.helper" | ||
| - "@wp.proxy" | ||
| - "@helper.nonce-verifier" | ||
| - "@wp.proxy" |
There was a problem hiding this comment.
🔵 Suggestion / Question: This duplicate "@wp.proxy" constructor argument is removed with no explanation, in contrast to the well-commented nonce fix elsewhere in this PR. PHP silently ignores extra constructor arguments, so it's not obvious this line was actually breaking anything at runtime - was this confirmed as part of the WP-1019 root cause, or is it unrelated cleanup bundled into this PR? Worth clarifying in the commit message/PR description either way.
|
|
||
| == Changelog == | ||
| = 5.7.4 = | ||
| * Fixed Translation Lock popup silently failing to save unlocked fields |
There was a problem hiding this comment.
🔵 Suggestion: This changelog entry undersells the fix - the bug rejected every Translation Lock save (locking or unlocking), not just "failing to save unlocked fields".
3 participants
No description provided.