rework upload, fix bulk submit (WP-1015) - #630
Open
vsolovei-smartling wants to merge 20 commits into
Open
Conversation
…WP-1015) The "Clone attachment" profile option flagged an attachment is_cloned=1 and deferred the actual clone to UploadJob's processCloning() poll (findSubmissionForCloning()). Cloning makes no Smartling API calls and is a synchronous local operation, so sendForTranslation() now calls cloneContent() directly at the point it already holds the submission, instead of deferring it to a separate unclaimed poll that raced under concurrent cron runs. The standalone clone-request feature (ContentRelationsHandler's formAction=clone) has no live trigger in either UI (the React tab bar has no clone tab, and the legacy jQuery form is permanently display:none with nothing that reveals it), so it's left untouched as a separate dead-code cleanup. findSubmissionForCloning() stays on SubmissionManager: QueueManagerTableWidget still uses it to detect a clone stuck from a crashed prior run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
claim() updated a row by id alone, never re-checking that it was still unclaimed (or stale) at write time. Two concurrent dequeue() calls that both selected the same unclaimed row could both succeed in claiming it, since nothing in the UPDATE's WHERE would make the second one lose - each is dispatched to Smartling independently, so a lost race meant a real duplicate upload, not just a local bookkeeping error. claim() now re-checks the same unclaimed-or-stale condition dequeue() selected on, in the same UPDATE that writes the new claim: InnoDB serializes concurrent writers to a row and re-evaluates the WHERE against current data, so only one concurrent claim can ever match. Also fixes a related bug this exposed: the old `!== false` check on the query result treated an UPDATE matching zero rows as success, since PHP's `0 !== false` is true. Affected-rows is checked with `> 0` now, so a lost race is correctly treated as "not claimed" instead of being handed out anyway. The stale-claim condition dequeue()'s SELECT already used is extracted into staleClaimCondition() and shared with claim(), so the two queries can't drift out of sync on what counts as an abandoned claim. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
UploadQueueManager::claim() is now a real compare-and-swap, so concurrent UploadJob runs can no longer double-process the same queue row. The account-level distributed lock (placeLockFlag()/dropLockFlag(), one Smartling API round trip per acquire, and another per renew - which processUploadQueue() does after every single processed item) was the only thing serializing concurrent runs before that fix; it's no longer needed for correctness here. Added JobAbstract::usesDistributedLock() (default true, unchanged for every other job) so a job can skip the acquireLock()/renewLock()/ releaseLock() calls while keeping the local throttle-cache check and the "no active profile" guard placeLockFlag() also does. UploadJob overrides it to false. Known side effect, discussed and accepted: QueueManagerTableWidget's "Running, please wait..." cell for the upload queue probes this same lock, so it will no longer report the upload job as running. A live-refreshing queue count is planned to replace that indicator, as a separate change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…P-1015) UploadJob no longer holds the distributed lock, so QueueManagerTableWidget's "Running, please wait..." indicator for the Upload row could never fire again - the trial acquireLock() call would always succeed. Rather than keep a pointless API round trip on every page load, the Upload row now renders a counter span that JS polls every second via a new AJAX endpoint, so the admin can see the queue actually draining instead of a lock-derived state that no longer means anything. Added UploadQueueCountController (wp_ajax_smartling_upload_queue_count), mirroring InstantTranslationController's shape: nonce + capability checked, returns UploadQueueManager::count(). Reuses the smartling_connector_ajax nonce already localized to smartling-connector-admin.js for this page, so no new nonce plumbing was needed. The other rows (Download, Check Status, Check Status Helper) are unchanged - they still hold the lock and still need the "Running" probe, so tests exercising that behavior were retargeted from the Upload row to Download rather than deleted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Only when the polled count actually differs from what's displayed: snap the text to a highlight color (a CSS custom property, --smartling-queue-count-highlight) with transitions disabled, force a reflow, then remove that class so the base rule's transition calmly fades the color back over 900ms. No flash/scale - just a color that settles back to normal, so repeated identical polls stay inert. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… (WP-1015) Wrapped the cell's link+counter state in a stable #smartling-upload-cron-cell span so JS can replace the whole cell, not just the counter. When a poll reports count === 0, the cell now swaps to the same "Nothing to do" text page load would render, and polling stops (the interval's own "element gone" check would have caught it a tick later regardless, but stopping immediately avoids one wasted request). Known simplification: page load's "Nothing to do" state also depends on findSubmissionForCloning() (a lingering pending clone), which the polled endpoint doesn't check. That state is rare now that cloning is synchronous (only a mid-clone crash leaves one behind) and reachable only via a fresh page load, same as before this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… flow (WP-1015) UserTranslationRequest::fromArray()'s description fallback mixed ?? and ?: without parens: `$array['description'] ?? count($ids) > 0 ? 'From Bulk Submit' : 'From Widget'` parses as `($array['description'] ?? (count($ids) > 0)) ? 'From Bulk Submit' : 'From Widget'`, since ?? binds tighter than ?:. Verified with a PHP repro: a real caller-supplied description was silently discarded and replaced by one of the two canned labels. Parenthesized the intended grouping instead. ContentRelationsDiscoveryService::clone() (the standalone "clone" formAction, reachable via ContentRelationsHandler::createSubmissionsHandler() regardless of UI exposure) stored submissions with isCloned=1/status=NEW and relied entirely on UploadJob::processCloning() to actually clone them. That executor was removed in an earlier commit on this branch, so clone() has been silently broken since: submissions it creates are now permanently stuck, never cloned, with no error surfaced. Removed clone() and ContentRelationsHandler's formAction=clone dispatch branch (FORM_ACTION_CLONE constant included) rather than fix an already-dead feature - the React tab bar has no clone tab (confirmed dead via its TabPanel definition), so nothing can reach this path. UserCloneRequest becomes pointless once clone() is gone: it was only a base class for UserTranslationRequest and a type hint on getSources(), which is only ever called with a UserTranslationRequest now. Merged its properties/getters into UserTranslationRequest directly and deleted the class, retyping getSources() accordingly. Also dropped the now-provably-dead 'clone' tab conditionals in js/app.js (tab.name can only ever be 'new'/'existing'/'instant'). inc/Smartling/WP/View/ContentEditJob.php's one remaining FORM_ACTION_CLONE reference is left untouched - that legacy jQuery view is documented as kept for backwards compatibility only and is already unreachable (wrapped in display:none). Found during code review of PR #630. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…dedup (WP-1015) Findings from code review of PR #630: - UploadQueueManager::delete() checked `!== false` instead of affected-rows > 0, the same pitfall claim() was already fixed for in this branch: a successful DELETE/UPDATE matching zero rows returns int(0), and `0 !== false` is true in PHP, so a lost race was reported as success. Now checked the same way as claim(). - ContentEditJob.php's legacy jQuery view still rendered a live Clone tab and #cloneButton wired to the create-submissions AJAX call with formAction hardcoded to upload, even though all backend clone handling was removed earlier on this branch. Removed the dead tab, button, and click-handler wiring; the view is otherwise unreachable (display:none) so this is a pure risk-reduction cleanup, not a behavior change. - The new upload-queue-count poller (js/smartling-connector-admin.js) had no .fail() handler, so a persistent AJAX error (nonce rotation, 5xx, network blip) left it silently polling admin-ajax.php once a second forever with no visible progress. Added a consecutive-failure counter that stops the interval after 5 failures. - The nonce + capability check block was duplicated near-verbatim across UploadQueueCountController and ContentRelationsHandler (2 handlers). Extracted into AjaxSecurityTrait::checkAjaxNonceAndCapability(), with failure reasons in a plain AjaxAuthorizationFailure class rather than trait constants, since trait constants require PHP 8.2 and this project targets PHP 8.0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…h (WP-1015) findSubmissionForCloning() had no remaining callers other than QueueManagerTableWidget's "Nothing to do" check, and existed only to support the old async cloning poll that sendForTranslation() replaced with a synchronous, in-place clone earlier on this branch. Deleted it along with the now-pointless WordpressFunctionProxyHelper dependency it was the only reason QueueManagerTableWidget (and its constructor wiring in ConfigurationProfilesController/services.yml) carried. InstantTranslationController's two AJAX handlers hand-rolled the same nonce+capability check AjaxSecurityTrait was introduced for elsewhere on this branch (ContentRelationsHandler, UploadQueueCountController), including a raw, non-proxied get_current_user_id() call. Switched both handlers to the shared trait for consistency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
processBulkAction()'s $action==='clone' branch created isCloned=1 submissions and deliberately excluded them from the upload queue, relying on UploadJob::processCloning() to clone them later. That executor was already removed earlier on this branch (its trigger, UploadJob's async cloning poll, was replaced with a synchronous clone in sendForTranslation()), so any submission built via this path was silently orphaned forever: isCloned=1, status stuck, no error, no retry. Removed the $clone branch from processBulkAction() - every submission is now always prepared as a normal upload and enqueued. prepareForUpload() lost its now-always-false bool $clone parameter, and always sets isCloned(0). Also removed the dead UI that was the only way to reach this: the 'Clone' tab, its locale checkboxes and button in BulkSubmit.php, the action=clone hidden field, and WPAbstract::bulkSubmitCloneButton() - same class of dead legacy jQuery markup (wrapped in display:none, superseded by the React #smartling-app widget) already cleaned up for ContentEditJob.php earlier on this branch. ACTION_SMARTLING_CLONE_CONTENT and its documentation were already fully removed by an earlier commit on this branch; verified zero remaining references anywhere in the repo, so no further action needed there. Found during code review of PR #630. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
processBulkAction() had no CSRF protection: any request carrying a submission/locale/job payload would prepare and enqueue uploads regardless of how it arrived. Add a nonce check gated on the presence of an actual bulk-action payload, verified via WordpressFunctionProxyHelper::wp_verify_nonce() for testability, and render the matching wp_nonce_field() in the Bulk Submit form. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…anslation lock, fix queue count capability (WP-1015) - BulkSubmitTableWidget: normalize submission/bulk-submit-locales/smartling request values to arrays before the CSRF guard, so a scalar value can no longer bypass the nonce check while still reaching retrieveBatch() or triggering a PHP 8 array_key_exists() TypeError. - SubmissionTableWidget: add the same nonce-verification pattern (and the same array normalization) to processBulkAction() on the Translation Progress page. - TranslationLockController: add nonce verification to handleFormPost(), which previously accepted POSTed lock/unlock changes with no CSRF protection at all. - UploadQueueCountController: gate the queue-count AJAX endpoint on SMARTLING_CAPABILITY_PROFILE_CAP instead of the widget capability, matching the capability required to view the page the counter is rendered on. - Wire WordpressFunctionProxyHelper through SubmissionsPageController and TranslationLockController (and their services.yml entries) so both can verify nonces the same way BulkSubmitTableWidget does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…load queue count poller race (WP-1015) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There is no way to clone a submission in recent versions of the plugin, so testTranslationAndCloningRelationsOneLevelDeep's cloning half (and the createSubmissionForCloning() helper, CLONE_BLOG_ID, ORIGINAL_BLOG_ID) tested a capability that's no longer reachable. This surfaced as a failure after processCloning() was removed from UploadJob: cloned submissions enqueued this way now sit at New forever, since nothing performs the clone anymore. Renamed to testTranslationRelationsOneLevelDeep and stripped to the translation-only assertions, which remain valid and unrelated to cloning. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same reasoning as the RelationsTest fix: there is no way to clone a submission in recent versions of the plugin, so tests that manually force is_cloned=1 to exercise the clone-completion path are testing something unreachable. Both now error with "Attempt to read property ... on null" since nothing ever completes the clone to create the target post being asserted on. - ClonePostWithImageAndTaxonomyTest::testComplexClone was the only test in the file and entirely about cloning (post + taxonomy + attachment, relation propagation) - removed the file. - CloneTest::testLocking used a cloned submission only as a vehicle to bootstrap target content before testing locked-fields behavior - removed it and its now-unused assertPostValues() helper. testIsClonedClearedOnTranslation() doesn't rely on a clone actually completing (only that the flag gets cleared by a normal translation upload) and is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sl-mmuradov
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.