Remove source locale from targets (WP-1016) - #631
Conversation
…P-1016) ConfigurationProfileFormController::save() set the source locale and built the target-locale list independently, and validateTargetLocales() only checked for duplicate Smartling locale codes among targets - never against the new source blogId. The only place that ever excluded the source locale from targets was a render-time loop in the profile edit view, which only affects what's displayed on the next page load, not what gets persisted when the source locale itself is changed in the same submission. Now save() skips any submitted target-locale entry whose blogId matches the profile's (possibly just-changed) source locale before persisting, so a stale or duplicate row can no longer be saved as both source and target.
…1016) Target-locale rows were previously skipped entirely from the DOM for whatever blog was the currently persisted source locale, so switching the source-locale select client-side (without reloading the page) never made the previous source available as a target, nor hid the newly picked source from the target list until the page was reloaded post-save. Every target-locale row is now always rendered, tagged with its blogId, and hidden/disabled by default only if it matches the profile's current source locale. A change handler on the source-locale selects keeps this in sync live: picking a new source hides and disables its row (so it can never be submitted as a target) and re-enables/reveals whatever was previously hidden - all without a page reload, on top of the save()-time filtering already in place as the source of truth.
…WP-1016) setAccessible() only became a no-op starting PHP 8.1; this project targets PHP 8.0, where it's still required to invoke a protected method via reflection.
…en (WP-1016) The Submissions Board previously showed the Title and Locale columns as plain text, with no way to jump to the actual WordPress content being translated. This links Title to the source content's edit screen and Locale to the target content's edit screen, both multisite-aware via get_admin_url($blogId, ...). - WordpressContentTypeHelper: extracted buildEditUrl() shared by the existing getEditUrl() (target) and new getSourceEditUrl() (source). getEditUrl()'s behavior for existing callers is unchanged. - SubmissionTableWidget: new buildSourceTitleCell()/buildTargetLocaleCell() helpers wrap the title/locale text in a link when one is available, falling back to plain text when the id is 0 (not yet translated) or the content type is unsupported. - Attachments resolve through post.php?post=ID&action=edit, same as regular posts. Built with TDD; full unit suite (625 tests) passes.
…ent types (WP-1016) buildEditUrl() called ContentTypeManager::getHandler($contentType) with no exception handling. getHandler() throws SmartlingInvalidFactoryArgumentException for any content type that isn't currently registered as a descriptor - e.g. a historical submission row whose custom-post-type integration has since been removed or deactivated (observed in production for 'sovos_product'). SubmissionTableWidget::prepare_items() calls getEditUrl()/getSourceEditUrl() for every row on the Submissions Board regardless of content type, so an unregistered historical content type crashed the entire page with an uncaught fatal error. buildEditUrl() now catches the exception, logs a warning (including the submission id for debugging), and returns '' - the same "no link available" fallback already used for unsupported/unknown content types, matching the graceful-degradation pattern already used by getLocalizedContentType() in this same class. Reproduced with a failing test first (mocking getHandler() to throw the same exception/message seen in production), then fixed. Full unit suite (627 tests) passes.
| Tested up to: 7.0 | ||
| Requires PHP: 8.0 | ||
| Stable tag: 5.7.0 | ||
| Stable tag: 5.7.3 |
There was a problem hiding this comment.
5.7.1 and 5.7.2 were development releases
PavelLoparev
left a comment
PavelLoparev
left a comment
There was a problem hiding this comment.
Automated review pass (Claude Code) on the WP-1016 fix.
Core fix: ConfigurationProfileFormController::save() now skips any submitted target-locale row whose blogId matches the current source locale before persisting - enforced server-side, so it can't be bypassed by disabled/hidden form fields or a direct POST. Good pairing with the live JS toggle in configuration-profile-form.js. Test coverage for this and the incidental fatal-error fix (buildEditUrl() catching getHandler() exceptions for unregistered/historical content types) is solid.
A few inline suggestions below - none blocking, but worth a look before release.
Ready to merge? With fixes (nice-to-haves, not regressions in the core logic).
| $status = $item->getStatusColor(); | ||
| $statusFlags = $item->getStatusFlags(); | ||
| $editUrl = WordpressContentTypeHelper::getEditUrl($item); | ||
| $editUrl = WordpressContentTypeHelper::getTargetEditUrl($item); |
There was a problem hiding this comment.
🟡 warning: unlike post-based-content-type.php (which guards this call with if (0 !== (int) $item->getTargetId())), this call to getTargetEditUrl($item) has no such guard - so when no target content exists yet, this produces a link like /term.php?taxonomy=category&tag_ID=0. Pre-existing, but since this PR is hardening the analogous call site in post-based-content-type.php, worth aligning here too:
if (0 !== (int) $item->getTargetId()) {
$editUrl = WordpressContentTypeHelper::getTargetEditUrl($item);
}There was a problem hiding this comment.
Fixed - added the same getTargetId() guard used in post-based-content-type.php.
| return $blogLabel; | ||
| } | ||
|
|
||
| return HtmlTagGeneratorHelper::tag('a', $blogLabel, ['href' => $targetEditUrl]); |
There was a problem hiding this comment.
🔵 suggestion: $blogLabel is interpolated unescaped into the <a> tag here, unlike buildSourceTitleCell above, whose caller pre-escapes the title with htmlentities(). Pre-existing gap, but this exact line is already being touched - cheap to close now, e.g. htmlentities($blogLabel) at the call site (line 432) or inside this method.
There was a problem hiding this comment.
Fixed - now escaping $blogLabel with htmlentities() at the call site, matching how buildSourceTitleCell's caller pre-escapes the title.
|
|
||
| == Changelog == | ||
| = 5.7.3 = | ||
| * Fixed possible misconfiguration where target locales were being saved as a duplicate of a newly changed source locale |
There was a problem hiding this comment.
🔵 suggestion: the 5.7.3 changelog entry only documents the source/target-locale collision fix. This PR also adds Submissions Board source/target edit links and fixes a fatal error on that same board for historical unregistered content types - neither is mentioned. Consider adding a bullet for each.
There was a problem hiding this comment.
Added changelog bullets for both the source/target edit links and the historical-content-type fatal error fix.
| // Bare, unproxied WP global called by WordpressContentTypeHelper::buildEditUrl(). | ||
| // Not stubbed anywhere else for the unit test suite. | ||
| if (!function_exists(__NAMESPACE__ . '\\get_admin_url')) { | ||
| function get_admin_url($blogId = null, $path = '') |
There was a problem hiding this comment.
🔵 suggestion: this defines a real, permanent Smartling\Helpers\get_admin_url() function (guarded by function_exists). Since PHP can't redefine functions, whichever test class runs first "wins" for the rest of the PHPUnit process if another test ever needs a different stub for this namespaced function - an ordering hazard given processIsolation="false". Worth considering WordpressFunctionProxyHelper (already used elsewhere in the codebase) instead of a bare global function.
There was a problem hiding this comment.
Leaving this as-is. WordpressContentTypeHelper::buildEditUrl() (and its getTarget/SourceEditUrl wrappers) are private static and called statically from plain-PHP view templates with no DI container in scope (e.g. taxonomy-based-content-type.php, post-based-content-type.php, SubmissionTableWidget.php), so routing this through the instance-based WordpressFunctionProxyHelper would mean a broader refactor of the helper and every call site - out of scope for this ticket. The bare-namespaced-function stub is also an established pattern already: ConfigurationProfileFormControllerTest.php does the same thing for its own namespace (Smartling\WP\Controller), so there's no actual collision with this one (Smartling\Helpers) today.
| if (array_key_exists('targetLocales', $settings)) { | ||
| $locales = []; | ||
|
|
||
| foreach ($settings['targetLocales'] as $blogId => $settings) { |
There was a problem hiding this comment.
🟣 question: this loop's $blogId => $settings reuses the $settings name, which shadows the outer &$_REQUEST['smartling_settings'] reference from the top of save() - each iteration overwrites $_REQUEST['smartling_settings'] with the current row's sub-array. Harmless today since $settings isn't read again after the loop, but it's a landmine for whoever adds code after this loop next. Pre-existing, flagging only because this exact block is being touched - might be worth a quick rename (e.g. $targetLocaleSettings) while in here.
There was a problem hiding this comment.
Fixed - renamed the loop variable to $targetLocaleSettings so it no longer shadows the outer $settings reference.
- guard getTargetEditUrl() call with getTargetId() check in taxonomy-based-content-type.php, matching post-based-content-type.php - htmlentities() the target blog label in SubmissionTableWidget, matching the source title cell's escaping - rename shadowed loop variable in ConfigurationProfileFormController::save() targetLocales loop - add changelog entries for the edit-links and fatal-error fixes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Also add links to the source and target content (if available) to the translation progress screen