rework upload, fix bulk submit (WP-1015)

composer.json

@@ -1,7 +1,7 @@
 {
   "name": "smartling/wordpress-connector",
   "license": "GPL-2.0-or-later",
-  "version": "5.6.2",
+  "version": "5.7.0",
   "description": "",
   "type": "wordpress-plugin",
   "repositories": [

css/smartling-connector-admin.css

@@ -455,3 +455,14 @@ span.circle {
 .smartling-border-table .numeric {
     text-align: right;
 }
+
+#smartling-upload-queue-count {
+    --smartling-queue-count-highlight: #2271b1;
+    color: inherit;
+    transition: color 900ms ease;
+}
+
+#smartling-upload-queue-count.smartling-queue-count-changed {
+    color: var(--smartling-queue-count-highlight);
+    transition: none;
+}

inc/Smartling/Base/ExportedAPI.php

@@ -68,12 +68,6 @@ interface ExportedAPI
      */
     public const ACTION_SMARTLING_SEND_FOR_TRANSLATION = 'smartling_send_for_translation';
 
-    /**
-     * Action that clones content of given SubmissionEntity without translation
-     * @param SubmissionEntity
-     */
-    public const ACTION_SMARTLING_CLONE_CONTENT = 'smartling_clone_content';
-
     /**
      * Action that downloads translation for given SubmissionEntity
      * @param SubmissionEntity

inc/Smartling/Base/SmartlingCore.php

@@ -40,7 +40,6 @@ public function __construct(
     ) {
         parent::__construct();
 
-        $this->wpProxy->add_action(ExportedAPI::ACTION_SMARTLING_CLONE_CONTENT, [$this, 'cloneContent']);
         $this->wpProxy->add_action(ExportedAPI::ACTION_SMARTLING_PREPARE_SUBMISSION_UPLOAD, [$this, 'prepareUpload']);
         $this->wpProxy->add_action(ExportedAPI::ACTION_SMARTLING_SEND_FOR_TRANSLATION, [$this, 'sendForTranslation']);
         $this->wpProxy->add_action(ExportedAPI::ACTION_SMARTLING_DOWNLOAD_TRANSLATION, [$this, 'downloadTranslationBySubmission',]);

inc/Smartling/Base/SmartlingCoreUploadTrait.php

@@ -532,7 +532,7 @@ public function sendForTranslation(UploadQueueItem $item): void
 
         $configurationProfile = $this->getSettingsManager()->getSingleSettingsProfile($item->getSubmissions()[0]->getSourceBlogId());
 
-        // Mark attachment submission as "Cloned" if there is "Clone attachment"
+        // Clone attachment submission instead of uploading it, if "Clone attachment"
         // option is enabled in configuration profile.
         foreach ($item->getSubmissions() as $submission) {
             if (1 === $configurationProfile->getCloneAttachment() && $submission->getContentType() === 'attachment') {
@@ -541,14 +541,32 @@ public function sendForTranslation(UploadQueueItem $item): void
 
                 $this->getLogger()->info(
                     sprintf(
-                        'Attachment submissionId="%s" marked as cloned (sourceBlogId="%s", sourceId="%s", contentType="%s", batchUid="%s").',
+                        'Cloning attachment submissionId="%s" (sourceBlogId="%s", sourceId="%s", contentType="%s", batchUid="%s").',
                         $submission->getId(),
                         $submission->getSourceBlogId(),
                         $submission->getSourceId(),
                         $submission->getContentType(),
                         $item->getBatchUid(),
                     )
                 );
+                try {
+                    $this->cloneContent($submission);
+                } catch (\Throwable $e) {
+                    // Marks the submission FAILED (see SubmissionManager::setErrorMessage()) -
+                    // terminal, not retried automatically. There is no longer a poll that would
+                    // pick a New-status cloned submission back up (that was UploadJob's removed
+                    // processCloning()), so a silent failure here would otherwise leave the
+                    // submission stuck invisibly. This matches how every other upload failure in
+                    // this method is handled: visible and requiring manual resubmission.
+                    $this->getSubmissionManager()->setErrorMessage(
+                        $submission, vsprintf('Error occurred while cloning: %s', [$e->getMessage()])
+                    );
+                    $this->getLogger()->error(sprintf(
+                        'Failed cloning attachment submissionId="%s": %s',
+                        $submission->getId(),
+                        $e->getMessage(),
+                    ));
+                }
                 $item = $item->removeSubmission($submission);
             }
         }
@@ -610,7 +628,7 @@ public function sendForTranslation(UploadQueueItem $item): void
         }
     }
 
-    public function prepareForUpload(string $contentType, int $sourceBlog, int $sourceEntity, int $targetBlog, JobEntityWithBatchUid $jobInfo, bool $clone): SubmissionEntity
+    public function prepareForUpload(string $contentType, int $sourceBlog, int $sourceEntity, int $targetBlog, JobEntityWithBatchUid $jobInfo): SubmissionEntity
     {
         $translationHelper = $this->getTranslationHelper();
         $submission = $translationHelper
@@ -629,8 +647,7 @@ public function prepareForUpload(string $contentType, int $sourceBlog, int $sour
             $submission->setStatus(SubmissionEntity::SUBMISSION_STATUS_NEW);
         }
 
-        $isCloned = true === $clone ? 1 : 0;
-        $submission->setIsCloned($isCloned);
+        $submission->setIsCloned(0);
         $submission->setJobInfo($jobInfo->getJobInformationEntity());
 
         return $this->getSubmissionManager()->storeEntity($submission);

inc/Smartling/DbAl/UploadQueueManager.php

@@ -50,19 +50,7 @@ public function dequeue(int $blogId): ?UploadQueueItem
         // Get queue items with the first submission having its source blog id = $blogId.
         // It should be impossible to create a single queue item with submissions from multiple source blog ids,
         // so only checking one is enough.
-        $staleClaimCondition = new ConditionBlock(ConditionBuilder::CONDITION_BLOCK_LEVEL_OPERATOR_OR);
-        $staleClaimCondition->addCondition(new Condition(
-            ConditionBuilder::CONDITION_IS_NULL,
-            'q.' . UploadQueueEntity::FIELD_CLAIMED,
-            [],
-            false,
-        ));
-        $staleClaimCondition->addCondition(new Condition(
-            ConditionBuilder::CONDITION_SIGN_LESS,
-            'q.' . UploadQueueEntity::FIELD_CLAIMED,
-            $this->getStaleClaimThreshold(),
-            false,
-        ));
+        $staleClaimCondition = $this->staleClaimCondition('q.');
 
         $query = sprintf(<<<'SQL'
 select q.%1$s, q.%2$s, q.%3$s, q.%9$s, q.%10$s from %7$s q left join %8$s s
@@ -171,6 +159,37 @@ private function getStaleClaimThreshold(): string
         );
     }
 
+    /**
+     * A row is eligible to be (re)claimed when nobody holds a claim on it, or the claim is
+     * old enough to have been abandoned by a crashed process.
+     *
+     * @param string $fieldPrefix Table alias prefix (e.g. 'q.') to use in a joined query.
+     *                            Left empty for an unqualified column reference.
+     */
+    private function staleClaimCondition(string $fieldPrefix = ''): ConditionBlock
+    {
+        // QueryBuilder::escapeName() wraps a bare column name in backticks, but would
+        // incorrectly wrap a full "alias.column" reference (e.g. 'q.claimed') the same
+        // way, producing invalid SQL - a prefixed reference must be used unescaped.
+        // Hence escaping only when there is no prefix to worry about.
+        $escapeField = $fieldPrefix === '';
+        $block = new ConditionBlock(ConditionBuilder::CONDITION_BLOCK_LEVEL_OPERATOR_OR);
+        $block->addCondition(new Condition(
+            ConditionBuilder::CONDITION_IS_NULL,
+            $fieldPrefix . UploadQueueEntity::FIELD_CLAIMED,
+            [],
+            $escapeField,
+        ));
+        $block->addCondition(new Condition(
+            ConditionBuilder::CONDITION_SIGN_LESS,
+            $fieldPrefix . UploadQueueEntity::FIELD_CLAIMED,
+            $this->getStaleClaimThreshold(),
+            $escapeField,
+        ));
+
+        return $block;
+    }
+
     public function complete(UploadQueueItem $item): void
     {
         if (!$this->delete($item->getId())) {
@@ -179,18 +198,27 @@ public function complete(UploadQueueItem $item): void
     }
 
     /**
+     * Claims a row by id, but only if it is still unclaimed (or stale) at the moment of the
+     * write. Matching by id alone would let two concurrent dequeue() calls that both selected
+     * the same unclaimed row both succeed in claiming it; re-checking the claim in the same
+     * UPDATE makes this a real compare-and-swap, since InnoDB serializes concurrent writers
+     * to the same row and re-evaluates the WHERE clause against the current data.
+     *
      * @return bool Whether the row was actually claimed.
      */
     private function claim(int $id, int $attempts): bool
     {
+        $conditions = $this->idCondition($id);
+        $conditions->addConditionBlock($this->staleClaimCondition());
+
         return $this->db->query(QueryBuilder::buildUpdateQuery(
             $this->tableName,
             [
                 UploadQueueEntity::FIELD_CLAIMED => DateTimeHelper::nowAsString(),
                 UploadQueueEntity::FIELD_ATTEMPTS => $attempts + 1,
             ],
-            $this->idCondition($id),
-        )) !== false;
+            $conditions,
+        )) > 0;
     }
 
     public function enqueue(IntegerIterator $submissionIds, string $batchUid): void
@@ -281,7 +309,10 @@ private function getSmartlingLocale(SubmissionEntity $submission): ?string
      */
     private function delete(int $id): bool
     {
-        return $this->db->query(QueryBuilder::buildDeleteQuery($this->tableName, $this->idCondition($id))) !== false;
+        // Affected-rows is checked with `> 0`, not `!== false`: a successful DELETE matching
+        // zero rows returns int(0), and `0 !== false` is true in PHP, which would report a
+        // no-op delete as success (see claim()'s equivalent check for the same pitfall).
+        return $this->db->query(QueryBuilder::buildDeleteQuery($this->tableName, $this->idCondition($id))) > 0;
     }
 
     private function idCondition(int $id): ConditionBlock

inc/Smartling/Helpers/AjaxAuthorizationFailure.php

@@ -0,0 +1,10 @@
+<?php
+
+namespace Smartling\Helpers;
+
+final class AjaxAuthorizationFailure
+{
+    public const INVALID_NONCE = 'invalid_nonce';
+
+    public const INSUFFICIENT_CAPABILITY = 'insufficient_capability';
+}

inc/Smartling/Helpers/AjaxSecurityChecker.php

@@ -0,0 +1,67 @@
+<?php
+
+namespace Smartling\Helpers;
+
+/**
+ * Shared nonce + capability check for WordPress AJAX handlers (wp_ajax_* actions),
+ * which combine both via check_ajax_referer(). For classes that render their own
+ * wp_nonce_field() and verify it directly (WP_List_Table bulk actions, form-post
+ * controllers), see NonceVerifier instead.
+ */
+class AjaxSecurityChecker
+{
+    use LoggerSafeTrait;
+
+    public function __construct(private WordpressFunctionProxyHelper $wpProxy)
+    {
+    }
+
+    /**
+     * @return string|null An AjaxAuthorizationFailure::* constant on failure, null when authorized.
+     */
+    public function check(string $nonceAction, string $capability, string $actionName): ?string
+    {
+        if ($this->wpProxy->check_ajax_referer($nonceAction, '_wpnonce', false) === false) {
+            $this->getLogger()->warning(sprintf(
+                'Invalid nonce for action "%s" from userId=%d',
+                $actionName,
+                $this->wpProxy->get_current_user_id(),
+            ));
+
+            return AjaxAuthorizationFailure::INVALID_NONCE;
+        }
+
+        if (!$this->wpProxy->current_user_can($capability)) {
+            $this->getLogger()->warning(sprintf(
+                'User %d lacks capability "%s" for action "%s"',
+                $this->wpProxy->get_current_user_id(),
+                $capability,
+                $actionName,
+            ));
+
+            return AjaxAuthorizationFailure::INSUFFICIENT_CAPABILITY;
+        }
+
+        return null;
+    }
+
+    /**
+     * @return bool Whether the request is authorized. When false, an error response has already been sent.
+     */
+    public function enforce(string $nonceAction, string $capability, string $actionName): bool
+    {
+        $authFailure = $this->check($nonceAction, $capability, $actionName);
+        if ($authFailure === AjaxAuthorizationFailure::INVALID_NONCE) {
+            $this->wpProxy->wp_send_json_error(['message' => 'Invalid nonce'], 403);
+
+            return false;
+        }
+        if ($authFailure === AjaxAuthorizationFailure::INSUFFICIENT_CAPABILITY) {
+            $this->wpProxy->wp_send_json_error(['message' => 'Insufficient permissions'], 403);
+
+            return false;
+        }
+
+        return true;
+    }
+}

inc/Smartling/Helpers/NonceVerifier.php

@@ -0,0 +1,24 @@
+<?php
+
+namespace Smartling\Helpers;
+
+/**
+ * Shared CSRF nonce verification for classes that render their own wp_nonce_field()
+ * and verify it directly against $_POST/$_REQUEST (WP_List_Table bulk actions,
+ * form-post controllers). For WordPress AJAX handlers, which combine nonce and
+ * capability checks via check_ajax_referer(), see AjaxSecurityChecker instead.
+ */
+class NonceVerifier
+{
+    public function __construct(private WordpressFunctionProxyHelper $wpProxy)
+    {
+    }
+
+    /**
+     * @param mixed $nonce Raw value read from the request; anything other than a non-empty string fails verification.
+     */
+    public function verify(mixed $nonce, string $nonceAction): bool
+    {
+        return is_string($nonce) && $nonce !== '' && false !== $this->wpProxy->wp_verify_nonce($nonce, $nonceAction);
+    }
+}

inc/Smartling/Helpers/WordpressFunctionProxyHelper.php

@@ -270,6 +270,11 @@ public function check_ajax_referer()
         return check_ajax_referer(...func_get_args());
     }
 
+    public function wp_verify_nonce()
+    {
+        return wp_verify_nonce(...func_get_args());
+    }
+
     public function current_user_can()
     {
         return current_user_can(...func_get_args());

inc/Smartling/Jobs/JobAbstract.php

@@ -85,6 +85,11 @@ protected function getCronFlagName(): string
         return self::CRON_FLAG_PREFIX . $this->getJobHookName();
     }
 
+    protected function usesDistributedLock(): bool
+    {
+        return true;
+    }
+
     /**
      * @throws EntityNotFoundException
      * @throws SmartlingApiException
@@ -117,6 +122,9 @@ public function placeLockFlag(bool $renew = false, string $source = ''): void
         if ($this->throttleIntervalSeconds > 0) {
             $this->cache->set($flagName, 1, $this->throttleIntervalSeconds);
         }
+        if (!$this->usesDistributedLock()) {
+            return;
+        }
         if ($renew) {
             $this->api->renewLock($profile, $flagName, $this->cronLockTtl);
         } else {
@@ -130,6 +138,9 @@ public function placeLockFlag(bool $renew = false, string $source = ''): void
      */
     public function dropLockFlag(): void
     {
+        if (!$this->usesDistributedLock()) {
+            return;
+        }
         $profile = $this->settingsManager->getActiveProfile();
         $flagName = $this->getCronFlagName();
         $this->getLogger()->debug(

inc/Smartling/Jobs/UploadJob.php

@@ -36,6 +36,11 @@ public function getJobHookName(): string
         return self::JOB_HOOK_NAME;
     }
 
+    protected function usesDistributedLock(): bool
+    {
+        return false;
+    }
+
     public function run(string $source): void
     {
         $message = 'UploadJob';
@@ -48,8 +53,6 @@ public function run(string $source): void
 
         $this->processUploadQueue($blogId);
 
-        $this->processCloning($blogId);
-
         $this->getLogger()->debug("Finished $message");
     }
 
@@ -124,17 +127,4 @@ private function failItem(UploadQueueItem $item, string $logVerb, string $errorM
             $this->submissionManager->setErrorMessage($submission, $errorMessage);
         }
     }
-
-    private function processCloning(int $blogId): void
-    {
-        while (($submission = $this->submissionManager->findSubmissionForCloning($blogId)) !== null) {
-            try {
-                $this->wpProxy->do_action(ExportedAPI::ACTION_SMARTLING_CLONE_CONTENT, $submission);
-            } catch (\Throwable $e) {
-                $this->submissionManager->setErrorMessage($submission, $e->getMessage());
-                continue;
-            }
-            $this->placeLockFlag(true);
-        }
-    }
 }