add fast translation (WP-985)

[vsolovei-smartling]

[PavelLoparev]

@vsolovei-smartling could you please setup claude code gh workflow like here?

[PavelLoparev]

🚨 Comprehensive Code Review Results

I've completed a comprehensive review of this PR using multiple specialized analyzers. Found 6 critical issues (bugs & security) and 4 important issues that should be addressed before merge.

Critical Issues Summary

1. ❌ Undefined variable bug in error handling (breaks error reporting)
2. ❌ Backoff algorithm bug (wastes API quota)
3. 🔒 Missing CSRF protection (security vulnerability)
4. 🔒 Missing permission checks (authorization vulnerability)
5. ❌ Missing return statements after JSON responses
6. 🔐 No input sanitization (WordPress standards violation)

Important Issues Summary

7. ⚠️ Silent API response failures (debugging impossible)
8. ⚠️ Batch processing hides partial failures
9. ⚠️ Overly broad exception catching
10. 📝 Test coverage gaps (0% on 290-line controller)

Detailed comments posted on specific lines below. Please address all critical issues before merge.

[PavelLoparev]

Critical Issue #1: Undefined Variable in Error Handling

File: inc/Smartling/FTS/FtsService.php (around line 719 in pollUntilComplete method)

Bug: The code references $data['error'] but the variable is named $response in this scope:

case self::STATE_FAILED:
    $error = $data['error'] ?? 'Translation request failed';  // ❌ $data is undefined

Impact: When a translation fails, this will cause a PHP notice/warning about an undefined variable. The actual error from the API will never be retrieved, and users will always see the generic fallback message.

Fix:

case self::STATE_FAILED:
    $error = $response['error'] ?? 'Translation request failed';  // ✅ Use $response
    $this->getLogger()->error(
        "Translation failed, submissionId={$submission->getId()}, " .
        "fileUid=$fileUid, mtUid=$mtUid, error=" . json_encode($error)
    );

Severity: 🔴 Critical - Breaks error reporting completely

[PavelLoparev]

Critical Issue #2: Exponential Backoff Never Increases

File: inc/Smartling/FTS/FtsService.php (pollUntilComplete method, lines ~694-738)

Bug: The $waitMs variable is reset to null at the top of every loop iteration, so the exponential backoff never actually backs off:

while (true) {
    $waitMs = null;  // ❌ Reset every iteration
    
    // ... polling logic ...
    
    $waitMs = $this->getNextPollInterval($waitMs);  // Always called with null
    usleep($waitMs * 1000);  // Always waits 1000ms
}

Impact: The polling will always wait exactly 1 second between requests instead of backing off to 2s, 4s, 8s, 16s, 30s. This wastes API quota and could trigger rate limiting.

Fix:

$waitMs = null;  // ✅ Initialize BEFORE the loop
while (true) {
    $elapsedMs = (int)((microtime(true) - $startTime) * 1000);
    
    // ... rest of polling logic (remove the $waitMs = null line) ...
    
    $waitMs = $this->getNextPollInterval($waitMs);
    usleep($waitMs * 1000);
}

Severity: 🔴 Critical - Defeats the purpose of exponential backoff

[PavelLoparev]

Critical Issue #3: Missing CSRF Protection

File: inc/Smartling/WP/Controller/InstantTranslationController.php
Methods: handleRequestTranslation() and handlePollStatus()

Security Vulnerability: Both AJAX endpoints read directly from $_POST without calling check_ajax_referer() or wp_verify_nonce().

Impact: These endpoints are vulnerable to Cross-Site Request Forgery (CSRF) attacks. A malicious website could trigger instant translations on behalf of an authenticated admin user.

Fix:

1. Add nonce verification at the start of both handlers:

public function handleRequestTranslation(): void
{
    check_ajax_referer('smartling_instant_translation', '_wpnonce');  // ✅ Add this
    
    try {
        $contentType = $_POST['contentType'] ?? '';
        // ... rest of method

2. Pass the nonce from JavaScript:

const response = await jQuery.post(ajaxUrl, {
    action: 'smartling_instant_translation',
    _wpnonce: smartlingData.nonce,  // ✅ Must be localized
    contentType: contentType,
    // ... rest of data
});

3. Localize the nonce in PHP (wherever JavaScript data is localized):

wp_localize_script('smartling-connector-admin', 'smartlingData', [
    'nonce' => wp_create_nonce('smartling_instant_translation'),  // ✅ Add this
    // ... other data
]);

Severity: 🔴 Critical - Security vulnerability (CSRF)

[PavelLoparev]

Critical Issue #4: Missing Capability/Permission Checks

File: inc/Smartling/WP/Controller/InstantTranslationController.php
Methods: handleRequestTranslation() and handlePollStatus()

Security Vulnerability: The endpoints are registered via wp_ajax_ (require login), but there's no current_user_can() check to verify the logged-in user has permission to trigger translations.

Impact: Any authenticated WordPress subscriber could invoke these endpoints and trigger translations, which should be restricted to users with appropriate capabilities (editors/admins).

Fix: Add at the start of both handlers (after nonce check):

public function handleRequestTranslation(): void
{
    check_ajax_referer('smartling_instant_translation', '_wpnonce');
    
    // ✅ Add capability check
    if (!current_user_can('publish_posts')) {
        $this->wpProxy->wp_send_json_error([
            'message' => 'Insufficient permissions'
        ], 403);
        return;
    }
    
    try {
        // ... rest of method

Note: Adjust the capability (publish_posts, edit_posts, etc.) based on your plugin's permission model.

Severity: 🔴 Critical - Security vulnerability (Authorization bypass)

[PavelLoparev]

Critical Issue #5: Missing Return Statements After JSON Responses

File: inc/Smartling/WP/Controller/InstantTranslationController.php

Bug: The code calls wp_send_json_error() and wp_send_json_success() through the proxy wrapper, but doesn't add return statements afterward. If the proxy implementation ever changes (e.g., for testing where wp_die is mocked), execution could continue after sending the response.

Example locations:

• After parameter validation (lines ~850)
• After various error conditions

Current code:

if (empty($contentType) || empty($contentId) || empty($targetBlogIds)) {
    $this->wpProxy->wp_send_json_error([
        'message' => 'Missing required parameters: contentType, contentId, or targetBlogIds'
    ], 400);
    // ❌ Execution continues here\!
}
$sourceBlogId = (int)get_current_blog_id();  // Could execute with invalid data

Fix: Add return; after every JSON response:

if (empty($contentType) || empty($contentId) || empty($targetBlogIds)) {
    $this->wpProxy->wp_send_json_error([
        'message' => 'Missing required parameters: contentType, contentId, or targetBlogIds'
    ], 400);
    return;  // ✅ Halt execution
}

Severity: 🔴 Critical - Defensive coding failure, could cause unexpected behavior

[PavelLoparev]

Critical Issue #6: No Input Sanitization

File: inc/Smartling/WP/Controller/InstantTranslationController.php

WordPress Standards Violation: WordPress coding standards require sanitizing all input. $_POST values are used directly without sanitize_text_field().

Current code:

$contentType = $_POST['contentType'] ?? '';
$contentId = (int)($_POST['contentId'] ?? 0);
$relations = $_POST['relations'] ?? [];
$targetBlogIds = array_map('intval', $_POST['targetBlogIds'] ?? []);

Issues:

• contentType could contain malicious characters
• relations array is completely unsanitized
• WordPress plugin review will reject code without proper sanitization

Fix:

$contentType = sanitize_text_field(wp_unslash($_POST['contentType'] ?? ''));
$contentId = (int)($_POST['contentId'] ?? 0);
$relations = map_deep(wp_unslash($_POST['relations'] ?? []), 'sanitize_text_field');
$targetBlogIds = array_map('intval', $_POST['targetBlogIds'] ?? []);

Also apply to handlePollStatus():

$submissionId = (int)($_POST['submissionId'] ?? 0);
// Add validation
if ($submissionId <= 0) {
    $this->wpProxy->wp_send_json_error(['message' => 'Invalid submission ID'], 400);
    return;
}

Severity: 🔴 Critical - WordPress standards violation, potential security risk

[PavelLoparev]

Important Issue #7: Silent API Response Failures

File: inc/Smartling/FTS/FtsApiWrapper.php (lines 354, 379)

Problem: When API calls fail or return unexpected responses, the code only throws generic exceptions without logging the actual API error details:

$fileUid = $this->getFileTranslationsApi($this->getConfigurationProfile($submission))
    ->uploadFile($filePath, $fileName, $fileType)['fileUid'] ?? null;

if (empty($fileUid)) {
    throw new \RuntimeException('Failed to get fileUid from upload response');
}

Hidden Errors:

• Network failures returning empty responses
• API authentication failures
• API error objects without 'fileUid' field
• HTTP 4xx/5xx status codes

Impact: When the API call fails, debugging is impossible because there's no information about what the API actually returned or why it failed.

Recommended Fix:

$response = $this->getFileTranslationsApi($this->getConfigurationProfile($submission))
    ->uploadFile($filePath, $fileName, $fileType);

$this->getLogger()->debug("Upload file API response: " . json_encode($response));

$fileUid = $response['fileUid'] ?? null;
if (empty($fileUid)) {
    $errorMessage = $response['error'] ?? $response['message'] ?? 'Unknown error';
    $this->getLogger()->error(
        "Failed to get fileUid from upload response, submissionId={$submission->getId()}, " .
        "apiResponse=" . json_encode($response)
    );
    throw new \RuntimeException("Failed to upload file for translation: $errorMessage");
}

Severity: ⚠️ High - Makes production debugging extremely difficult

[PavelLoparev]

Important Issue #8: Batch Processing Hides Partial Failures

File: inc/Smartling/FTS/FtsService.php (requestInstantTranslationBatch method, lines ~566-577)

Problem: When downloading and applying translations in batch mode, individual submission failures are logged but the overall operation still reports success:

foreach ($submissions as $submission) {
    try {
        $this->downloadAndApply($submission, $fileUid, $mtUid);
        $this->getLogger()->info("Translation applied for submission {$submission->getId()}");
    } catch (\Exception $e) {
        $this->getLogger()->error(
            "Failed to apply translation for submission {$submission->getId()}: {$e->getMessage()}"
        );
        $submission->setStatus(SubmissionEntity::SUBMISSION_STATUS_FAILED);
        $submission->setLastError($e->getMessage());
        $this->submissionManager->storeEntity($submission);
    }
}
// ❌ Returns success: true even if some submissions failed!
return [
    'success' => true,
    'status' => self::STATE_COMPLETED,
    ...
];

Impact: Users are told the batch operation succeeded when some translations actually failed. They won't know to check individual submission statuses.

Recommended Fix: Track and report partial failures:

$succeededSubmissions = [];
$failedSubmissions = [];

foreach ($submissions as $submission) {
    try {
        $this->downloadAndApply($submission, $fileUid, $mtUid);
        $succeededSubmissions[] = $submission->getId();
    } catch (\Exception $e) {
        // ... existing error handling ...
        $failedSubmissions[] = [
            'id' => $submission->getId(),
            'error' => $e->getMessage()
        ];
    }
}

$allSucceeded = empty($failedSubmissions);

return [
    'success' => $allSucceeded,
    'status' => $allSucceeded ? self::STATE_COMPLETED : 'partial_success',
    'succeeded' => $succeededSubmissions,
    'failed' => $failedSubmissions,
    'fileUid' => $fileUid,
    'mtUid' => $mtUid,
];

Severity: ⚠️ High - Users receive incorrect status information

[PavelLoparev]

Important Issue #9: Overly Broad Exception Catching

Files: Multiple locations in FtsService.php and InstantTranslationController.php

Problem: The code catches all \Exception types without distinguishing between expected errors (API failures, validation errors) and unexpected errors (programming bugs, resource exhaustion):

} catch (\Exception $e) {
    $this->getLogger()->error("Instant translation failed with exception: " . $e->getMessage());
    
    return [
        'success' => false,
        'status' => 'error',
        'message' => $e->getMessage(),
    ];
}

Hidden Errors:

• PHP fatal errors (TypeError, ArgumentCountError)
• Memory exhaustion errors
• Database connection failures
• Logic errors (DivisionByZeroError)

Impact: When an unexpected programming error occurs (like a TypeError due to a bug), it's caught and returned as if it were an expected API failure. This masks bugs and prevents proper bug reporting.

Recommended Fix: Catch specific expected exceptions separately:

} catch (SmartlingApiException $e) {
    $this->getLogger()->error("API error: " . $e->getMessage());
    return [
        'success' => false,
        'status' => 'api_error',
        'message' => 'Translation API error: ' . $e->getMessage(),
    ];
} catch (SmartlingDbException $e) {
    $this->getLogger()->error("Database error: " . $e->getMessage());
    return [
        'success' => false,
        'status' => 'database_error',
        'message' => 'Database error. Please contact support.',
    ];
} catch (\RuntimeException $e) {
    $this->getLogger()->error("Runtime error: " . $e->getMessage());
    return [
        'success' => false,
        'status' => 'error',
        'message' => $e->getMessage(),
    ];
} catch (\Throwable $e) {
    // Unexpected error - log with high severity for bug tracking
    $this->getLogger()->error(
        "UNEXPECTED ERROR, type=" . get_class($e) . ", message=" . $e->getMessage()
    );
    throw $e; // Re-throw to ensure it's not silently handled
}

Severity: ⚠️ High - Masks programming bugs as normal failures

[PavelLoparev]

Important Issue #10: Critical Test Coverage Gaps

Summary: The PR adds 834 lines of production code but only 220 lines of tests, with 0% coverage on the 290-line AJAX controller.

Missing Critical Tests:

InstantTranslationController - Completely Untested (290 lines)

• ❌ Request parameter validation (missing/invalid parameters)
• ❌ Exception handling in handleRequestTranslation
• ❌ JSON response format validation
• ❌ Status polling edge cases (invalid IDs, deleted submissions)
• ❌ Related content processing with circular references

FtsService Error Handling

• ❌ Polling state machine edge cases (STATE_FAILED, STATE_CANCELLED, unknown states)
• ❌ File upload cleanup on failure
• ❌ Batch processing partial failures
• ❌ Timeout handling
• ❌ Target locale resolution failures

FtsApiWrapper

• ❌ API response validation (missing fields, wrong types)
• ❌ Exception propagation
• ❌ Retry logic integration

Integration Tests

• ❌ No end-to-end test for complete workflow
• ❌ Backoff algorithm usage in polling loop
• ❌ Submission state transitions

Impact: The undefined $data variable bug (Issue #1) would have been caught by basic tests of the failure path. Current tests only cover happy paths and configuration errors.

Estimated Additional Test Lines Needed: 600-800 lines to achieve adequate coverage of critical paths.

Recommendation: At minimum, add tests for InstantTranslationController and error paths in FtsService.pollUntilComplete() before merge.

Severity: ⚠️ High - Insufficient quality assurance for production deployment