store configuration profile for delivery (WP-1021) by vsolovei-smartling · Pull Request #636 · Smartling/wordpress-localization-plugin

vsolovei-smartling · 2026-10-02T08:31:33Z

No description provided.

PavelLoparev

Issues (binary files — no inline diff available for comments)

inc/.DS_Store: 🔴 critical
Accidental developer-machine artifact (.DS_Store) committed. This permanently bloats the repo once merged (history can't be shrunk without a rewrite) and has no place in inc/.

Fix: remove this file from the PR and add .DS_Store to .gitignore.

inc/composer-backups.zip: 🔴 critical
A ~12MB Composer vendor backup zip was committed to inc/. This permanently bloats repo size/history and isn't part of the plugin source.

Fix: remove this file from the PR and add a backup/zip pattern to .gitignore.

Recommendations

The blog-id → profile-id cache-key swap was applied thoroughly (~12 call sites) but missed at least one spot (UploadQueueManager::purge). Worth grepping for any remaining getSourceBlogId()-keyed caches before merge.
Consider splitting stampConfigurationProfile semantics: "stamp if absent" for passive lookups vs. "stamp/refresh" for actual resubmit actions, rather than one method with resubmit-only intent living inside a generic lookup method.

Assessment

Ready to merge? With fixes
Reasoning: Core design (storing the profile id at request time, falling back to the active profile) is sound and well tested, but the UploadQueueManager::purge() cache-key miss will route batch-file cancellation to the wrong Smartling project for some submissions, and the committed composer-backups.zip/.DS_Store must not ship. The getSubmissionEntity re-stamping scope should be confirmed with the author before merge.

PavelLoparev · 2026-10-05T09:50:47Z

                if (!array_key_exists($submission->getSourceBlogId(), $profiles)) {
                    try {
-                        $profile = $this->settingsManager->getSingleSettingsProfile($submission->getSourceBlogId());
+                        $profile = $this->settingsManager->getProfileBySubmission($submission);


🔴 critical
purge()'s per-item profile cache is still keyed only by $submission->getSourceBlogId(), but getProfileBySubmission() can now return different profiles for submissions sharing the same source blog — that's the whole point of this feature. The first submission processed for a blog "wins" the cache, so every subsequent submission from that blog gets cancelBatchFile() called against the wrong profile/project, silently corrupting or failing the cancel.

UploadJob.php was correctly updated to key its analogous cache by profile id ($submission->getConfigurationProfileId() ?? "blog{$submission->getSourceBlogId()}") — this call site was missed.

Fix:

$profileKey = $submission->getConfigurationProfileId() ?? "blog{$submission->getSourceBlogId()}"; if (!array_key_exists($profileKey, $profiles)) { // ... $profiles[$profileKey] = $profile; } $profile = $profiles[$profileKey];

Fixed — keyed the cache by getConfigurationProfileId() ?? "blog{sourceBlogId}", matching UploadJob.php. Added a regression test (testPurgeUsesDistinctProfilePerSubmissionSharingSourceBlog) that fails without the fix. Grepped for other getSourceBlogId()-keyed profile caches; the only other hit (BlogRemovalHandler) builds its map directly from getEntities()/target locales rather than getProfileBySubmission(), so it isn't affected by this bug class.

PavelLoparev · 2026-10-05T09:50:47Z

            $entity->setSourceTitle('no title');
            $entity->setCreatedAt(DateTimeHelper::nowAsString());
        }
+        $this->stampConfigurationProfile($entity);


🟡 warning
stampConfigurationProfile($entity) is called unconditionally here for both newly-created and pre-existing submissions returned by getSubmissionEntity(). Since this is a general-purpose "find or create" helper used from multiple call sites (e.g. TranslationHelper::prepareSubmissionEntity, ContentRelationsDiscoveryService::bulkUpload), any caller that persists the returned entity for an unrelated reason will also silently overwrite the originally-recorded configuration_profile_id with whatever profile is currently active — the exact in-flight-submission scenario this feature (WP-1021) is meant to protect against.

The included test testGetSubmissionEntityRefreshesProfileOnExistingSubmission documents this as intentional — can you confirm every caller of getSubmissionEntity represents a genuine new/resubmitted translation request, not just an existence check?

If not, consider only stamping when the entity is newly created ($entity->getId() falsy before storeEntity), or move the stamping to call sites that truly mean "(re)send for translation".

Checked every caller of getSubmissionEntity():

TranslationHelper::prepareSubmissionEntity → prepareForUpload/prepareSubmission, both genuine upload/resubmit entry points.

ContentRelationsDiscoveryService::bulkUpload, a resubmit flow.

TranslationHelper::getExistingSubmissionOrCreateNew, called from PostBasedWidgetControllerStd's bulk-submit handler right before the submission is stamped NEW and stored for translation.

SubmissionManager::storeEntity's own internal dedup fallback (line ~400), which only fires when inserting a brand-new entity that collides with an existing row — also a genuine (re)submit, not a passive lookup.

Note the existence-check path (isRelatedSubmissionCreationNeeded → submissionExists/submissionExistsNoLastError) is already separate and does not call getSubmissionEntity(), so it isn't affected. Confirmed intentional as implemented.

🔴 critical
The caller audit covers getSubmissionEntity(), but the same "stamp always" semantics were also added to UploadJob::processUploadQueue() (UploadJob.php:83) and SmartlingCoreUploadTrait::prepareUpload() (line 64). Those run at cron/upload time, not request time, and overwrite the profile stamped when the user requested translation.

The batch, however, is created at request time with the then-active profile (ContentRelationsDiscoveryService::createSubmissions() → createBatch($profile, …), BulkSubmitTableWidget line 269), and the queue item carries that batchUid.

Scenario (the GoFundMe workflow): submit with A → batch in project A, submission stamped A → switch to B before the upload cron fires → UploadJob/prepareUpload re-stamp B → upload is sent with project B credentials against project A's batchUid (fails), and later delivery is routed to B.

The ticket asks to store the profile "at the time the user requests translation". Suggest: keep the refresh in getSubmissionEntity() (explicit (re)submit), stamp at enqueue time for paths that bypass it, and in UploadJob/prepareUpload only stamp when getConfigurationProfileId() === null. Test: stored A, active B, run UploadJob → still A.

🟡 warning
There is also a passive path the audit doesn't cover, because it goes through prepareUpload() rather than getSubmissionEntity(): ContentProvider::getContent() (ContentProvider.php:66) → getXMLFiltered() → prepareUpload() → stampConfigurationProfile() + renewContentHash() → storeEntity().

So simply fetching a submission's content XML silently rebinds an in-flight submission to the currently active profile and persists it, which is the existence-check/lookup case raised above. Stamping in prepareUpload() only when getConfigurationProfileId() === null would avoid it.

Confirmed and fixed — both call sites now only stamp when getConfigurationProfileId() === null:

UploadJob::processUploadQueue() — and now stamps every submission in the queue item, not just [0] (see the separate warning below).

SmartlingCoreUploadTrait::prepareUpload().

Also stamped explicitly at enqueue time in ContentRelationsDiscoveryService::createSubmissions()/bulkUpload(), for the submissions built there that bypass getSubmissionEntity() entirely (existing-submission reuse and submissionFactory->fromArray()) — otherwise they'd rely on whichever of the two conditional-stamp sites above happens to touch them first, which could already be after a profile switch.

Added your suggested test (stored A, active B, run UploadJob → still A) plus the equivalent for prepareUpload().

Fixed by the same change — prepareUpload() now only stamps when getConfigurationProfileId() === null, so a read-only getXMLFiltered() call via ContentProvider::getContent() no longer rebinds an in-flight submission. Added a test confirming an already-stamped submission survives a prepareUpload() call untouched.

PavelLoparev · 2026-10-05T09:50:47Z

+    /**
+     * Returns the profile the submission was requested with, so delivery doesn't depend on which profile is active now.
+     * Falls back to the active profile of the source blog for submissions without a stored (or an existing) profile.
+     *
+     * @throws SmartlingDbException
+     */
+    public function getProfileBySubmission(SubmissionEntity $submission): ConfigurationProfileEntity
+    {
+        $profileId = $submission->getConfigurationProfileId();
+        if ($profileId !== null) {
+            $profile = ArrayHelper::first($this->getEntityById($profileId));
+            if ($profile instanceof ConfigurationProfileEntity) {
+                return $profile;
+            }
+            $this->getLogger()->warning("Profile id=$profileId stored for submission id={$submission->getId()} not found, using active profile of source blog");
+        }
+
+        return $this->getSingleSettingsProfile($submission->getSourceBlogId());
+    }


🟡 warning
getProfileBySubmission() doesn't validate that the restored profile (getEntityById($profileId)) still belongs to the submission's source blog. If a configuration profile's original_blog_id is changed after a submission was stamped (profile repurposed for a different main blog), this will happily return it anyway. Narrow edge case, but worth at least a debug/warning log so a locale/project mismatch downstream is diagnosable.

Added a warning log when the restored profile's source blog no longer matches the submission's source blog, so a repurposed profile is diagnosable downstream.

PavelLoparev · 2026-10-05T09:50:47Z

            static::FIELD_LAST_ERROR => static::DB_TYPE_STRING_TEXT,
            static::FIELD_LOCKED_FIELDS => 'TEXT NULL',
            static::FIELD_CREATED_AT => static::DB_TYPE_DATETIME,
+            static::FIELD_CONFIGURATION_PROFILE_ID => 'INT(20) UNSIGNED NULL',


🔵 suggestion
New column uses the raw literal 'INT(20) UNSIGNED NULL' instead of composing from the existing DB_TYPE_U_BIGINT-style constants used by every other field in SmartlingEntityAbstract (DB_TYPE_U_BIGINT = 'INT(20) UNSIGNED NOT NULL'). Consider adding a DB_TYPE_U_BIGINT_NULL constant for consistency.

Added DB_TYPE_U_BIGINT_NULL next to DB_TYPE_U_BIGINT in SmartlingEntityAbstract and switched the column definition to use it.

PavelLoparev · 2026-10-05T09:50:47Z

                ),
                $this->contentSerializationHelper->prepareFieldProcessorValues($submission)['ignore'],
-                $this->settingsManager->getSingleSettingsProfile($submission->getSourceBlogId())->getFilterFieldNameRegExp()),
+                $this->settingsManager->getProfileBySubmission($submission)->getFilterFieldNameRegExp()),


🔵 suggestion
getProfileBySubmission($submission) is now called 2-3 times back-to-back for the same $submission within single methods (also at lines 138 & 142 in processStringsBeforeEncoding, and 183 & 187 in filterArray), each doing its own getEntityById round-trip when a profile id is stamped. Consider resolving the profile once per method and reusing it:

$profile = $this->settingsManager->getProfileBySubmission($submission); // ... $profile->getFilterFieldNameRegExp()

Resolved getFilterFieldNameRegExp() once per method in both processStringsBeforeEncoding and filterArray and reused it.

PavelLoparev · 2026-10-05T09:50:47Z

+ * Stores the configuration profile a submission was requested with.
+ *
+ * Existing rows are left NULL on purpose: the profile active today is not necessarily the
+ * one they were uploaded with, so they keep resolving the profile by source blog.


🟣 question
This docblock says existing rows are intentionally left NULL so they keep resolving via getSingleSettingsProfile($sourceBlogId). But given SubmissionManager::getSubmissionEntity() now re-stamps configuration_profile_id on every lookup (not just creation — see the comment on SubmissionManager.php:489), won't most pre-existing in-flight submissions get backfilled with the currently active profile the first time any lookup touches them, rather than staying NULL as this comment implies? Worth confirming this matches intent.

Confirmed this matches intent, per the caller audit above: every getSubmissionEntity() call site is a genuine (re)submit, so a pre-existing in-flight submission does get backfilled with the currently active profile the first time it's touched by one of those flows — it won't stay NULL indefinitely. Will tighten the docblock to say rows start NULL and get backfilled on first (re)submission rather than implying they stay NULL.

PavelLoparev · 2026-10-05T09:50:47Z

+WP_DB_HOST="${WP_DB_HOST:-127.0.0.1}"
+WP_DB_NAME="${WP_DB_NAME:-wordpress_test}"
+WP_DB_TABLE_PREFIX="${WP_DB_TABLE_PREFIX:-wptests_}"
+WP_INSTALL_DIR="${WP_INSTALL_DIR:-/opt/homebrew/var/www}"


🔵 suggestion
This script hardcodes a macOS Homebrew default path (/opt/homebrew/var/www) and dumps a developer's local "production" WordPress DB into the test DB — it reads as a personal convenience script rather than shared tooling. Consider documenting it as explicitly local/optional with no expectation of portability across contributors' machines, or moving the default out of the script into .env.local.example.

Added a header note that this is a personal/local convenience script with no portability guarantee, and made it fail fast with a pointer to .env.local.example if WP_INSTALL_DIR isn't set, instead of silently falling back to a Homebrew path.

…-1021) Record the profile used at translation request time on the submission and resolve it from there for upload, download and FTS, so switching the active profile no longer breaks automated delivery. Submissions without a stored profile fall back to the active profile of the source blog. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

…P-1021) Queued uploads load existing submissions by id and never went through SubmissionManager::getSubmissionEntity(), so configuration_profile_id stayed null. Stamp and persist the profile in UploadJob and prepareUpload. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

- fix UploadQueueManager::purge() caching profiles by source blog id instead of configuration profile id, which could cancel batch files against the wrong project when submissions from the same blog were stamped with different profiles - log a warning in SettingsManager::getProfileBySubmission() when a stored profile's source blog no longer matches the submission's source blog, so a repurposed profile is diagnosable - add DB_TYPE_U_BIGINT_NULL and use it for the new configuration profile id column instead of a raw literal - resolve the profile once per method in FieldsFilterHelper instead of calling getProfileBySubmission() repeatedly for the same submission - document tests/setup-local-test-db.sh as a personal/local convenience script and require WP_INSTALL_DIR instead of silently defaulting to a Homebrew path - remove inc/.DS_Store and inc/composer-backups.zip, rewritten out of branch history entirely rather than just deleted in a new commit, and ignore both patterns going forward

vsolovei-smartling · 2026-10-05T10:22:01Z

Addressed the binary-artifact findings: inc/.DS_Store and inc/composer-backups.zip (~12MB) are removed. Since the branch wasn't merged yet, I rewrote c27695cc to drop them entirely rather than adding a new commit on top, so the blob never enters master's history. Added .DS_Store and *-backups.zip to .gitignore.

Also addressed the recommendations:

UploadQueueManager::purge() cache-key fix, with a regression test (see reply on that thread).
Grepped for other getSourceBlogId()-keyed profile caches — only other hit was BlogRemovalHandler, which isn't affected (builds its map from getEntities(), not getProfileBySubmission()).
Left stampConfigurationProfile as "stamp always" rather than splitting semantics — audited every getSubmissionEntity() caller and all are genuine (re)submit flows, not passive existence checks (see reply on that thread for the full list).

sl-mmuradov · 2026-10-05T10:42:15Z

    private function getConfigurationProfile(SubmissionEntity $submission): ConfigurationProfileEntity
    {
-        $profile = $this->settings->getSingleSettingsProfile($submission->getSourceBlogId());
+        $profile = $this->settings->getProfileBySubmission($submission);


🔴 critical
Credentials/project now come from the stored profile, but the locale passed to the API still comes from the active profile: SettingsManager::getSmartlingLocaleBySubmission() (SettingsManager.php:95) still calls getSingleSettingsProfile($submission->getSourceBlogId()). It is used by downloadFile() (line 189) and getStatus() (line 215) here, plus UploadQueueManager::getSmartlingLocale() and SmartlingCoreTrait.

Scenario: submission stamped with profile A (project A, target blog 2 → de-DE), user switches active profile to B (blog 2 → de, or blog 2 not mapped). Status check / download for A's file goes to project A with B's locale, or throws SmartlingConfigException → automated delivery fails, which is exactly the WP-1021 case.

Fix:

public function getSmartlingLocaleBySubmission(SubmissionEntity $submission): string { $profile = $this->getProfileBySubmission($submission); // ...

plus a test with stored ≠ active profile.

Fixed — getSmartlingLocaleBySubmission() now calls getProfileBySubmission() instead of getSingleSettingsProfile(), matching ApiWrapper::getConfigurationProfile(). It falls back to the active profile exactly the same way getProfileBySubmission() already does for the no-stamp/deleted-profile cases, so downloadFile()/getStatus()/UploadQueueManager::getSmartlingLocale()/SmartlingCoreTrait all pick this up automatically. Added a test with stored profile A (locale de-DE) vs. active profile B, asserting A's locale is used and getSingleSettingsProfile() is never called.

sl-mmuradov · 2026-10-05T10:42:15Z

        }

        $settings = $this->contentSerializationHelper->prepareFieldProcessorValues($submission);
+        $filterFieldNameRegExp = $this->settingsManager->getProfileBySubmission($submission)->getFilterFieldNameRegExp();


🟡 warning
The regexp flag now comes from the stored profile, but the ignore/copy lists in $settings (line above) still come from the active one: ContentSerializationHelper::prepareFieldProcessorValues() (ContentSerializationHelper.php:90) uses findEntityByMainLocale($submission->getSourceBlogId()). After a profile switch, a single upload/apply mixes filter settings from two profiles (skip list / copy-by-name / SEO keys from B, regexp mode from A).

Fix: resolve the profile in prepareFieldProcessorValues() via getProfileBySubmission() (keeping the empty-filter fallback on SmartlingDbException).

Fixed — ContentSerializationHelper::prepareFieldProcessorValues() now resolves the profile via getProfileBySubmission() instead of findEntityByMainLocale(), with the filter left at empty defaults on SmartlingDbException (same fallback behavior as before, just profile-aware). Added tests for both the stamped-profile-is-used case and the exception fallback.

sl-mmuradov · 2026-10-05T10:42:15Z

+            // SubmissionManager::getSubmissionEntity(): remember the profile used for this upload here.
+            $previousProfileId = $submission->getConfigurationProfileId();
+            $this->submissionManager->stampConfigurationProfile($submission);
+            if ($submission->getConfigurationProfileId() !== $previousProfileId) {


🟡 warning
Only $item->getSubmissions()[0] is stamped/stored here, while the item groups submissions for several target locales that share one batch. The others only get stamped later inside prepareUpload() during sendForTranslation; if that fails before reaching them, they keep NULL/stale profile ids and can be delivered via a different profile than the first one.

Suggest stamping all submissions of the item (or stamping at enqueue time — see the thread on SubmissionManager.php:489).

Fixed — UploadJob::processUploadQueue() now loops over every submission in $item->getSubmissions() and stamps each one individually (only if not already stamped), instead of only [0]. Added a test with one already-stamped and one unstamped submission in the same item, asserting the unstamped one gets stamped and the already-stamped one is left untouched.

- SmartlingUnitTestCaseAbstract::getLogger() was type-hinted to the real Psr\Log\LoggerInterface, but MonologWrapper::getLogger() returns LevelLogger, which implements the scoped Smartling\Vendor\Psr\Log\LoggerInterface. Calling it threw a TypeError; nothing had called it before this test did. - testFullFtsWorkflow exercises the real upload/serialization pipeline (ContentHelper, FieldsFilterHelper's metadata filters) without registering WordPress hooks first, so FILTER_SMARTLING_METADATA_PROCESS_BEFORE_TRANSLATION had no listener and apply_filters() returned its "value" argument (the submission) unchanged, corrupting serialized field values. Added $this->loadBuiltInFilters(), matching the pattern already used by RelationsTest for the same reason. Also applied the already-committed Migration261001 to the local integration test database (tests/setup-local-test-db.sh predates this migration); no code change needed for that part.

SmartlingCoreTest::setUp() constructed SmartlingCore with a real WordpressFunctionProxyHelper. SmartlingCore::__construct() registers several add_action/add_filter hooks bound to $this. Under the unit-only test run (bootstrap_units.php) this is harmless because add_filter is a mocked no-op function, but when the full suite runs under the real WP bootstrap (as CI's Buildplan/test.sh does: both the "plugin test" and "integration test" suites in one `phpunit -c tests/phpunit.xml` process, with no --bootstrap override), these become genuine WordPress hook registrations that persist in global state for the rest of the process, one per test method, each bound to a SmartlingCore instance that never went through the DI container's setContentHelper()/etc. calls. FtsIntegrationTest::testFullFtsWorkflow is the first test to actually fire FILTER_SMARTLING_PREPARE_TARGET_CONTENT in that combined run. The stale SmartlingCoreTest callbacks run first and call getContentHelper() on their own un-wired instance, throwing "Typed property ContentHelper:: $ioFactory must not be accessed before initialization" before the real, properly-wired entrypoint singleton's callback ever runs - reproduced locally with `phpunit -c tests/phpunit.xml` (both suites, no bootstrap override) and confirmed fixed by this change. Mock only add_action/add_filter (matching the existing pattern in SmartlingCoreTraitTest, which already avoids this) so the hooks SmartlingCore's constructor registers become no-ops, while every other proxy method still delegates to the real/mocked WP functions the rest of this test file's assertions rely on.

Root cause of the remaining FtsIntegrationTest::testFullFtsWorkflow failure ("Target post was not created"): smartling-connector.php only hooks Bootstrap::load() onto 'plugins_loaded' when is_admin() || DOING_CRON, neither of which is true under the PHPUnit CLI bootstrap, so it never runs there on its own - a test has to call it explicitly (as RelationsTest and FtsIntegrationTest already do via loadBuiltInFilters()). That explicit call is not enough on its own, though. WP_UnitTestCase::setUp() snapshots $wp_filter the first time any test runs and tearDown() restores that exact snapshot after every test (_backup_hooks()/_restore_hooks()), unconditionally wiping anything a test registered. SmartlingCore registers several hooks (including FILTER_SMARTLING_PREPARE_TARGET_CONTENT) in its own constructor, and since it's a container-cached singleton, that constructor - and the add_filter() calls inside it - only ever runs once for the whole process. Whichever test happens to trigger that first construction gets working hooks for its own duration, then loses them at its own tearDown; every later test (including this PR's new FtsIntegrationTest, which never got the lucky first slot once the earlier SmartlingCoreTest fix removed it as a construction trigger) is left with zero listeners on a hook its own filter-chain code depends on, with apply_filters() silently returning the submission unchanged and no target ever created. Loading the plugin here, before wp-settings.php's plugin loading has even returned control to the test runner and before any test's setUp() takes that first $wp_filter snapshot, means the snapshot itself already includes every hook the plugin registers, so later tests calling loadBuiltInFilters() again is harmless (idempotent) rather than their only chance at a working hook. Verified by reproducing the exact failure with `phpunit -c tests/phpunit.xml` (both testsuites, real WP bootstrap, no --testsuite/--bootstrap override - matching Buildplan/test.sh), adding temporary instrumentation to confirm zero listeners were registered on FILTER_SMARTLING_PREPARE_TARGET_CONTENT at the moment of failure, and confirming the fix resolves it across both a dirty and a freshly rebuilt local test database.

wpCliExec() passed WP_INSTALL_DIR to wp-cli's --path, which is the developer's own real WordPress install (tests/.env.local.example's documented default), not the dedicated test install created by tests/setup-local-test-db.sh (WPCLI_PATH, whose wp-config.php points at the isolated test database). Every integration test that drives a translation through the real cron queue (runCronTask() -> wpCliExec('cron', 'event', 'run ...')) was running that cron event against the wrong database, so the job never saw the queued submission and silently did nothing. This only affects local (non-Docker) runs: Buildplan/test.sh and docker-init.sh never set WPCLI_PATH, so getWPcliPathEnv() falls back to WP_INSTALL_DIR there, unchanged. Discovered while trying to reproduce a CI-reported MetadataPartialVsFullTest failure locally: that test uses the same cron path and was failing with an unrelated, local-only symptom (target content never created) until this fix.

Root cause of MetadataPartialVsFullTest::testTranslatePostWithMetadata failing with "array doesn't have key 'meta_a'": confirmed via bisection this is a genuine regression (passes cleanly on master under identical conditions), introduced by WP-1021's first commit. SmartlingUnitTestCaseAbstract::cleanUpTables() truncates the posts and smartling_submissions tables before every test, so every test's own post/submission fixtures restart from auto-increment id 1. ContentHelper::readSourceMetadata() caches by "{contentType}-{sourceBlogId}-{sourceId}" in RuntimeCacheHelper, a process-wide singleton that persists for the whole PHPUnit run and is never cleared between tests. So whichever earlier test's "submission 1 / post 1" first triggers an upload (populating the cache with *its* metadata) silently serves that stale entry to every later test that also lands on id 1 - which, thanks to the truncation, is effectively every integration test exercising this path. Confirmed by instrumenting readSourceMetadata() to log its cache key, hit/miss state and call stack: MetadataPartialVsFullTest's own submission is id=1/sourceId=1, and an earlier test's upload (SmartlingCore::prepareTargetEntity -> readSourceContentWithMetadataAsArray) had already populated that exact cache key first. WP-1021 doesn't touch this caching path at all; it just happened to shift which test first reaches that code for id 1, surfacing a pre-existing cross-test contamination bug in the harness. Clearing the cache alongside the table truncation in setUp() fixes it without touching any production caching behavior (WP admin requests build a fresh container, and therefore a fresh RuntimeCacheHelper, every time).

SmartlingUnitTestCaseAbstract::cleanUpTables() truncates 'smartling_queue' (the legacy Queue class's download queue table) but never UploadQueueEntity's own table, smartling_upload_queue. Since posts and smartling_submissions ARE truncated every test (resetting their auto-increment ids back to 1), a stale upload_queue row left over from an earlier test's incomplete upload references a submission id that gets silently reused by whatever later test next creates submission #1 - and UploadQueueManager::dequeue() resolves queue rows by id, so it finds and reprocesses a submission that has nothing to do with the row's original test. Found this while investigating target content not being created for MetadataPartialVsFullTest/SubmissionUploadTest after real upload/download cycles: instrumented UploadJob::processUploadQueue() and saw the upload queue length balloon to 32 by the time that test ran, with the same submission id being repeatedly reclaimed across multiple cron invocations. Confirmed via the same instrumentation that queue length returns to 1 per invocation once this table is included.

sl-mmuradov found several places where the profile stamped at request time (the whole point of WP-1021) got silently discarded or inconsistently applied downstream: - UploadJob::processUploadQueue() and SmartlingCoreUploadTrait::prepareUpload() unconditionally re-stamped on every run, overwriting the profile active when the batch was created with whatever is active when the cron job/upload happens to fire. Both now only stamp when getConfigurationProfileId() is still null. UploadJob now does this for every submission in the queue item, not just the first - the others previously stayed unstamped until sendForTranslation touched them individually, with no stamp at all if that failed first. - prepareUpload() runs on every getXMLFiltered() call, including read-only content fetches (ContentProvider::getContent()), so the unconditional re-stamp also meant simply fetching a submission's XML silently rebound it to whatever profile is currently active. - ContentRelationsDiscoveryService::createSubmissions()/bulkUpload() build submissions directly (existing-submission reuse, and submissionFactory->fromArray()) without ever going through SubmissionManager::getSubmissionEntity(), so they never got stamped at all. Both paths now stamp explicitly with the profile the request's batch is being created under, at the moment of the request - not deferred to whichever downstream code first happens to touch the submission. - SettingsManager::getSmartlingLocaleBySubmission() still resolved the *active* profile (getSingleSettingsProfile) even though ApiWrapper::getConfigurationProfile() already resolves the *stamped* one (getProfileBySubmission) for credentials/project. After a profile switch this could send the right project with the wrong (or a nonexistent) locale. Now uses getProfileBySubmission() too, falling back to the active profile exactly like getConfigurationProfile() does. - ContentSerializationHelper::prepareFieldProcessorValues() resolved ignore/copy/SEO filter lists via findEntityByMainLocale() (always the active profile), while FieldsFilterHelper's regexp-mode flag already came from the stamped profile. After a profile switch this mixed filter settings from two different profiles in one upload/apply. Now uses getProfileBySubmission(), keeping the empty-filter fallback for SmartlingDbException. Added unit test coverage for each of these - previously prepareUpload(), getSmartlingLocaleBySubmission(), and prepareFieldProcessorValues() had none at all, and the enqueue-time-stamping paths in ContentRelationsDiscoveryService were only ever exercised against a fully-mocked SubmissionManager.

vsolovei-smartling · 2026-10-05T18:51:02Z

Addressed @sl-mmuradov's round of review comments — these identified real gaps where the profile stamped at request time (the whole point of WP-1021) was getting silently overwritten or inconsistently applied downstream:

UploadJob/prepareUpload() unconditionally re-stamping on every run (overwriting the request-time profile with whatever's active when the cron job fires) — now conditional on getConfigurationProfileId() === null.
Same unconditional re-stamp meant read-only content fetches (ContentProvider::getContent()) silently rebound submissions — fixed by the same change.
UploadJob only stamped $item->getSubmissions()[0], leaving the rest of a multi-locale batch unstamped — now stamps every submission in the item.
ContentRelationsDiscoveryService's direct submission-construction paths (bypassing getSubmissionEntity()) never stamped at all — now stamp explicitly at enqueue time with the profile the batch is created under.
SettingsManager::getSmartlingLocaleBySubmission() still resolved the active profile while credentials already came from the stamped one — now both use getProfileBySubmission().
ContentSerializationHelper::prepareFieldProcessorValues() resolved ignore/copy/SEO filter lists from the active profile while the regexp-mode flag came from the stamped one — now both come from the same (stamped) profile.

Added unit test coverage for all of these — several of the affected methods (prepareUpload(), getSmartlingLocaleBySubmission(), prepareFieldProcessorValues()) had no direct test coverage before. Full unit suite (723 tests) and the integration suite pass.

vsolovei-smartling requested review from PavelLoparev and sl-mmuradov October 5, 2026 09:18

PavelLoparev reviewed Oct 5, 2026

View reviewed changes

vsolovei-smartling and others added 3 commits October 5, 2026 12:16

vsolovei-smartling force-pushed the WP-1021-store-profile-for-delivery branch from e3b34e1 to b8e4092 Compare October 5, 2026 10:20

clarify Migration261001 docblock on profile backfill timing (WP-1021)

06bdd03

sl-mmuradov reviewed Oct 5, 2026

View reviewed changes

vsolovei-smartling added 7 commits October 5, 2026 13:15

vsolovei-smartling merged commit 86323f9 into master Oct 5, 2026
3 checks passed

vsolovei-smartling deleted the WP-1021-store-profile-for-delivery branch October 5, 2026 19:17

Conversation

vsolovei-smartling commented Oct 2, 2026

Uh oh!

PavelLoparev left a comment

Choose a reason for hiding this comment

Issues (binary files — no inline diff available for comments)

Recommendations

Assessment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsolovei-smartling commented Oct 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsolovei-smartling commented Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants