Adding the ability to use machine translation providers to the copy feature #106
Conversation
WalkthroughAdds provider-based auto-translation: new provider interfaces and implementations (Google, DeepL), a ProviderFactory, MLAutoTranslate trait with flatten/expand/translate logic, JS modal-based provider selection and auto-translate flows, updates form widgets and frontend controls to support provider-aware copy/translate, config entries, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Widget as Form Widget UI
participant Modal as Provider Modal
participant MultiJS as multiLingual JS
participant Backend as Server Handler
participant Provider as Translation Provider
User->>Widget: Click "copy locale"
Widget->>MultiJS: emit copyLocale (object)
MultiJS->>Modal: show provider selection modal
User->>Modal: choose provider -> click copy
Modal->>Widget: copyLocale payload w/ provider
alt provider == "standard" or none
Widget->>Widget: apply copyFromValue locally
else provider specified
Widget->>Backend: POST copyFromValue, copyFromLocale, currentLocale, provider
Backend->>Backend: MLAutoTranslate::autoTranslateArray()
Backend->>Provider: translate(texts[], targetLocale, sourceLocale)
Provider-->>Backend: translated texts[]
Backend-->>Widget: response with translatedValue
Widget->>Widget: apply translated content and update locale
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
formwidgets/MLBlocks.php (1)
108-139: Add error handling for auto-translation failures.The auto-translation call (lines 116-121) can throw exceptions from network failures, API errors, or invalid provider configurations. Without error handling, these failures will break the entire copy operation and show users generic error messages.
Apply this diff to add graceful error handling:
public function onCopyItemLocale() { $copyFromLocale = post('_blocks_copy_locale'); $currentLocale = post('_blocks_current_locale'); $provider = post('_provider'); $copyFromValues = $this->getLocaleSaveDataAsArray($copyFromLocale); if ($provider !== 'standard' && !empty($copyFromValues)) { + try { $copyFromValues = $this->autoTranslateArray( $copyFromValues, $currentLocale, $copyFromLocale, $provider ); + } catch (\Exception $e) { + \Log::error('Auto-translation failed: ' . $e->getMessage()); + // Continue with untranslated values + } } $this->reprocessLocaleItems($copyFromValues);
🧹 Nitpick comments (17)
traits/mlcontrol/partials/_locale_copy.htm (2)
46-48: Provider display name uses basic ucfirst transformation.Using
ucfirst($providerKey)for display names may not work well for multi-word provider keys (e.g., "deepl_pro" would display as "Deepl_pro" instead of "DeepL Pro").Consider using a display name from the provider configuration or a dedicated language key:
- <?= ucfirst($providerKey) ?> + <?= e($providerConfig['label'] ?? ucfirst($providerKey)) ?>Then add a
labelkey to each provider configuration in config/config.php.
52-56: HTML structure places modal-footer inside fieldset.The modal-footer is nested inside the fieldset element, which is semantically unusual. Typically, modal-footer should be a direct child of modal-content, after modal-body.
Consider restructuring the HTML to follow Bootstrap modal conventions:
- <div class="modal-footer"> - <button data-selected-locale="" type="button" class="btn btn-primary" - data-dismiss="modal"><?= e(trans('winter.translate::lang.locale.copy_button')); ?></button> - </div> </fieldset> </div> </form> </div> + <div class="modal-footer"> + <button data-selected-locale="" type="button" class="btn btn-primary" + data-dismiss="modal"><?= e(trans('winter.translate::lang.locale.copy_button')); ?></button> + </div> </div> </div> </div>contracts/TranslationProvider.php (1)
5-12: Consider documenting batch size limitations.The interface accepts an array of strings but doesn't specify any limitations on batch size. Different translation providers may have different limits (e.g., Google Translate has a 128-text limit per request, DeepL has character limits).
Consider adding documentation about batch handling expectations:
interface TranslationProvider { /** + * Translate multiple text strings from source to target locale. + * + * Implementations may have batch size limitations and should handle + * chunking internally if needed. + * * @param string[] $input * @return string[] translated output */assets/css/multilingual.css (1)
1-1: Avoid !important flags if possible.The
.ml-modal labelrule uses!importanton both properties, which suggests specificity conflicts with other styles. This makes the styles harder to override and maintain.Consider increasing selector specificity instead of using
!important:-.ml-modal label{color:#333 !important;text-transform:none !important} +.ml-modal .modal-body label, +.ml-modal .radio label{color:#333;text-transform:none}Verify that this approach resolves the styling conflicts by testing the modal UI in the browser.
formwidgets/MLNestedForm.php (1)
101-112: Consider adding error handling for translation failures.The
autoTranslateArraycall lacks error handling. If the translation provider fails (network issues, API errors, invalid credentials), this could result in an unhandled exception that disrupts the user workflow.Consider wrapping the translation call with error handling:
$copyFromValues = $this->getLocaleSaveDataAsArray($copyFromLocale); if ($provider !== 'standard' && !empty($copyFromValues)) { - $copyFromValues = $this->autoTranslateArray( - $copyFromValues, - $currentLocale, - $copyFromLocale, - $provider - ); + try { + $copyFromValues = $this->autoTranslateArray( + $copyFromValues, + $currentLocale, + $copyFromLocale, + $provider + ); + } catch (\Exception $e) { + // Log error and fall back to standard copy + \Log::error('Auto-translation failed: ' . $e->getMessage()); + // copyFromValues remains unchanged, performing standard copy + } }formwidgets/MLText.php (1)
6-6: Remove unused import.The
Exceptionclass is imported but never used in this file. Remove the unused import to keep the code clean.-use Exception;formwidgets/MLRepeater.php (1)
111-122: Consider adding error handling for translation failures.Similar to MLNestedForm, the
autoTranslateArraycall lacks error handling. Translation provider failures could result in unhandled exceptions.Consider wrapping the translation call with error handling:
$copyFromValues = $this->getLocaleSaveDataAsArray($copyFromLocale); if ($provider !== 'standard' && !empty($copyFromValues)) { - $copyFromValues = $this->autoTranslateArray( - $copyFromValues, - $currentLocale, - $copyFromLocale, - $provider - ); + try { + $copyFromValues = $this->autoTranslateArray( + $copyFromValues, + $currentLocale, + $copyFromLocale, + $provider + ); + } catch (\Exception $e) { + // Log error and fall back to standard copy + \Log::error('Auto-translation failed: ' . $e->getMessage()); + // copyFromValues remains unchanged, performing standard copy + } }assets/less/multilingual.less (1)
7-10: Consider avoiding !important flags.Using
!importantfor bothcolorandtext-transformsuggests a CSS specificity conflict. Consider addressing the root cause by increasing selector specificity or restructuring the CSS cascade instead of forcing precedence with!important.For example, you could increase specificity:
.ml-modal.ml-modal label { color: #333; text-transform: none; }Or target the modal more specifically if it has a unique context.
ProviderFactory.php (1)
14-18: Consider validating provider configuration.While the config check ensures the provider key exists, it doesn't validate that required fields (url, key) are present, which could lead to runtime errors when providers attempt translation.
Consider adding validation:
$config = Config::get("winter.translate::providers.$provider"); if (!$config) { throw new \Exception("No provider found: $provider"); } + +if (empty($config['url']) || empty($config['key'])) { + throw new \Exception("Provider '$provider' is missing required configuration (url or key)"); +}providers/GoogleTranslateProvider.php (1)
26-30: API key in URL query string may be logged.Passing the API key in the URL query string means it could be logged in server access logs, load balancers, and proxy logs. While this is how Google's API works, consider documenting this security consideration.
formwidgets/mlricheditor/assets/js/mlricheditor.js (1)
96-109: LGTM with minor suggestion: Auto-translation flow is well-structured.The two-step copy-then-translate flow is clean, and event handling is properly managed. Consider adding a check for undefined translatedValue on line 104.
Optional improvement:
MLRichEditor.prototype.onAutoTranslateSuccess = function(e, data) { const translatedValue = data.translatedValue[0] - if (typeof translatedValue != 'string' || !this.$richeditor.data('oc.richEditor')) { + if (!translatedValue || typeof translatedValue != 'string' || !this.$richeditor.data('oc.richEditor')) { return } this.$richeditor.richEditor('setContent', translatedValue); }config/config.php (1)
85-98: LGTM: Environment-driven provider configuration.Using environment variables for API credentials follows security best practices. Consider adding comments documenting the required environment variables for developers.
Optional: Add inline comments documenting the environment variables:
+ // Required environment variables: + // TRANSLATE_PROVIDER: 'google' or 'deepl' (default: 'google') + // GOOGLE_TRANSLATE_KEY: Your Google Cloud API key + // DEEPL_API_KEY: Your DeepL API key 'defaultProvider' => env('TRANSLATE_PROVIDER', 'google'),providers/DeepLTranslateProvider.php (1)
34-36: Consider more specific exception types.Throwing a generic
Exceptionmakes it harder for callers to distinguish between different failure modes (authentication, network, invalid locale, etc.). Consider using custom exception types or at least including the HTTP status code in the message.if (!$response->successful()) { - throw new Exception("DeepL Translation failed: " . $response->body()); + throw new Exception("DeepL Translation failed (HTTP {$response->status()}): " . $response->body()); }formwidgets/mltext/assets/js/mltext.js (1)
31-35: Remove incomplete comment and unused code.Lines 33-34 contain an incomplete comment (
// switchHan) and a commented-out default locale option that should be cleaned up.Apply this diff:
MLText.DEFAULTS = { autoTranslateHandler: null, - // switchHan - // defaultLocale: 'en' }traits/MLAutoTranslate.php (3)
85-111: Improve error messages for missing data.The exception messages are minimal. Consider providing more context about what was expected vs. what was received to aid debugging.
Apply this diff:
if (!$copyFromLocale || !$currentLocale) { - throw new Exception("Missing locale selection"); + throw new Exception("Missing locale selection. copyFromLocale: " . var_export($copyFromLocale, true) . ", currentLocale: " . var_export($currentLocale, true)); } if (!$copyFromValue) { - throw new Exception("Nothing to translate"); + throw new Exception("Nothing to translate. copyFromValue is empty."); }
112-124: Validate provider configuration structure.The method retrieves provider config but doesn't validate that required keys (like 'url' and 'key') exist, which could lead to errors downstream in the provider implementations.
public function getProviderConfig($provider = "") { if ($provider == "") { $provider = Config::get('winter.translate::defaultProvider'); } $providerConfig = Config::get('winter.translate::providers.' . $provider); if (!$providerConfig) { throw new Exception("No config for provider: " . $provider); } + + if (!isset($providerConfig['url']) || !isset($providerConfig['key'])) { + throw new Exception("Provider config missing required keys (url, key): " . $provider); + } + return $providerConfig; }
134-143: Consider empty input array handling.The translate method doesn't validate that
$inputis non-empty. Providers may handle empty arrays differently, potentially causing unnecessary API calls or errors.public function translate($input, string $targetLocale, string $currentLocale, string $provider = "") { + if (empty($input)) { + return []; + } + if ($provider === '') { $provider = Config::get('winter.translate::defaultProvider'); } $translator = ProviderFactory::create($provider); return $translator->translate($input, $targetLocale, $currentLocale); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
ProviderFactory.php(1 hunks)assets/css/multilingual.css(2 hunks)assets/js/multilingual.js(4 hunks)assets/less/multilingual.less(7 hunks)composer.json(1 hunks)config/config.php(1 hunks)contracts/TranslationProvider.php(1 hunks)formwidgets/MLBlocks.php(2 hunks)formwidgets/MLMarkdownEditor.php(1 hunks)formwidgets/MLNestedForm.php(2 hunks)formwidgets/MLRepeater.php(2 hunks)formwidgets/MLRichEditor.php(1 hunks)formwidgets/MLText.php(3 hunks)formwidgets/MLTextarea.php(1 hunks)formwidgets/mlblocks/assets/js/mlblocks.js(2 hunks)formwidgets/mlmarkdowneditor/assets/js/mlmarkdowneditor.js(3 hunks)formwidgets/mlmarkdowneditor/partials/_mlmarkdowneditor.htm(1 hunks)formwidgets/mlnestedform/assets/js/mlnestedform.js(2 hunks)formwidgets/mlrepeater/assets/js/mlrepeater.js(2 hunks)formwidgets/mlricheditor/assets/js/mlricheditor.js(3 hunks)formwidgets/mlricheditor/partials/_mlricheditor.htm(1 hunks)formwidgets/mltext/assets/js/mltext.js(1 hunks)formwidgets/mltext/partials/_mltext.htm(1 hunks)formwidgets/mltextarea/assets/js/mltextarea.js(1 hunks)formwidgets/mltextarea/partials/_mltextarea.htm(1 hunks)lang/en/lang.php(1 hunks)providers/DeepLTranslateProvider.php(1 hunks)providers/GoogleTranslateProvider.php(1 hunks)tests/unit/traits/MLAutoTranslateTest.php(1 hunks)traits/MLAutoTranslate.php(1 hunks)traits/MLControl.php(5 hunks)traits/mlcontrol/partials/_locale_copy.htm(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
ProviderFactory.php (2)
providers/GoogleTranslateProvider.php (1)
GoogleTranslateProvider(9-45)providers/DeepLTranslateProvider.php (1)
DeepLTranslateProvider(9-42)
contracts/TranslationProvider.php (3)
providers/DeepLTranslateProvider.php (1)
translate(18-41)providers/GoogleTranslateProvider.php (1)
translate(18-44)traits/MLAutoTranslate.php (1)
translate(134-143)
formwidgets/mlricheditor/assets/js/mlricheditor.js (4)
formwidgets/MLRichEditor.php (1)
MLRichEditor(15-108)assets/js/multilingual.js (1)
provider(164-164)formwidgets/mlmarkdowneditor/assets/js/mlmarkdowneditor.js (1)
translatedValue(99-99)formwidgets/mltext/assets/js/mltext.js (1)
translatedValue(62-62)
formwidgets/MLRepeater.php (4)
assets/js/multilingual.js (1)
provider(164-164)formwidgets/MLBlocks.php (1)
getLocaleSaveDataAsArray(208-217)formwidgets/MLNestedForm.php (1)
getLocaleSaveDataAsArray(183-192)traits/MLAutoTranslate.php (1)
autoTranslateArray(70-83)
formwidgets/MLNestedForm.php (3)
assets/js/multilingual.js (1)
provider(164-164)formwidgets/MLRepeater.php (1)
getLocaleSaveDataAsArray(207-216)traits/MLAutoTranslate.php (1)
autoTranslateArray(70-83)
formwidgets/MLBlocks.php (3)
assets/js/multilingual.js (1)
provider(164-164)formwidgets/MLNestedForm.php (1)
getLocaleSaveDataAsArray(183-192)traits/MLAutoTranslate.php (1)
autoTranslateArray(70-83)
traits/MLControl.php (1)
classes/TranslatableBehavior.php (2)
setTranslatableUseFallback(156-161)getAttributeTranslated(186-231)
providers/GoogleTranslateProvider.php (3)
providers/DeepLTranslateProvider.php (2)
__construct(13-16)translate(18-41)contracts/TranslationProvider.php (1)
translate(11-11)traits/MLAutoTranslate.php (1)
translate(134-143)
formwidgets/mltext/assets/js/mltext.js (3)
formwidgets/MLText.php (1)
MLText(15-64)formwidgets/mlmarkdowneditor/assets/js/mlmarkdowneditor.js (1)
translatedValue(99-99)formwidgets/mlricheditor/assets/js/mlricheditor.js (1)
translatedValue(104-104)
formwidgets/mlmarkdowneditor/assets/js/mlmarkdowneditor.js (4)
formwidgets/MLMarkdownEditor.php (1)
MLMarkdownEditor(15-100)assets/js/multilingual.js (1)
provider(164-164)formwidgets/mlricheditor/assets/js/mlricheditor.js (1)
translatedValue(104-104)formwidgets/mltext/assets/js/mltext.js (1)
translatedValue(62-62)
formwidgets/mltextarea/assets/js/mltextarea.js (5)
formwidgets/MLTextarea.php (1)
MLTextarea(14-69)assets/js/multilingual.js (1)
provider(164-164)formwidgets/mlmarkdowneditor/assets/js/mlmarkdowneditor.js (1)
translatedValue(99-99)formwidgets/mlricheditor/assets/js/mlricheditor.js (1)
translatedValue(104-104)formwidgets/mltext/assets/js/mltext.js (1)
translatedValue(62-62)
formwidgets/mlrepeater/assets/js/mlrepeater.js (2)
formwidgets/MLRepeater.php (1)
MLRepeater(18-247)assets/js/multilingual.js (1)
provider(164-164)
traits/MLAutoTranslate.php (4)
ProviderFactory.php (2)
ProviderFactory(10-26)create(12-25)contracts/TranslationProvider.php (1)
translate(11-11)providers/DeepLTranslateProvider.php (1)
translate(18-41)providers/GoogleTranslateProvider.php (1)
translate(18-44)
providers/DeepLTranslateProvider.php (3)
providers/GoogleTranslateProvider.php (1)
translate(18-44)contracts/TranslationProvider.php (1)
translate(11-11)traits/MLAutoTranslate.php (1)
translate(134-143)
formwidgets/mlnestedform/assets/js/mlnestedform.js (2)
formwidgets/MLNestedForm.php (1)
MLNestedForm(15-223)assets/js/multilingual.js (1)
provider(164-164)
formwidgets/mlblocks/assets/js/mlblocks.js (2)
formwidgets/MLBlocks.php (1)
MLBlocks(18-248)assets/js/multilingual.js (1)
provider(164-164)
tests/unit/traits/MLAutoTranslateTest.php (5)
tests/TranslatePluginTestCase.php (1)
TranslatePluginTestCase(13-18)contracts/TranslationProvider.php (1)
translate(11-11)providers/DeepLTranslateProvider.php (1)
translate(18-41)providers/GoogleTranslateProvider.php (1)
translate(18-44)traits/MLAutoTranslate.php (3)
translate(134-143)flatten(24-40)expand(50-68)
🪛 GitHub Actions: Tests
ProviderFactory.php
[error] 21-21: Parse error: Unexpected '=>' (T_DOUBLE_ARROW) due to PHP match expression used with PHP 7.4. Upgrade PHP to at least 8.0 or refactor to a compatible switch/if structure.
composer.json
[error] 1-1: Composer dependency resolution failed. Your requirements could not be resolved to an installable set of packages. Constraint mismatch: root requires winter/wn-backend-module dev-develop, >=1.2.8, but available versions do not satisfy this constraint. Use 'composer update -W' to allow upgrades/downgrades for locked packages.
tests/unit/traits/MLAutoTranslateTest.php
[error] 26-26: Class "GuzzleHttp\Psr7\Response" not found. (Occurred during: php artisan winter:test -p Winter.Translate)
[error] 50-50: Class "GuzzleHttp\Psr7\Response" not found. (Occurred during: php artisan winter:test -p Winter.Translate)
[error] 96-96: Class "GuzzleHttp\Psr7\Response" not found. (Occurred during: php artisan winter:test -p Winter.Translate)
[error] 119-119: Class "GuzzleHttp\Psr7\Response" not found. (Occurred during: php artisan winter:test -p Winter.Translate)
[error] 158-158: Class "GuzzleHttp\Psr7\Response" not found. (Occurred during: php artisan winter:test -p Winter.Translate)
🔇 Additional comments (37)
composer.json (2)
42-46: Good addition of allow-plugins configuration.The
allow-pluginsconfiguration is a Composer 2.2+ best practice that explicitly allows thecomposer/installersplugin to execute.
1-47: Pipeline failure is unrelated to your changes.The pipeline failure indicating "root requires winter/wn-backend-module dev-develop" appears to be a CI environment configuration issue, not caused by the changes in this PR. The added Guzzle dependency and allow-plugins configuration are both appropriate.
You may need to work with the maintainers to resolve the CI configuration issue.
traits/mlcontrol/partials/_locale_copy.htm (1)
17-62: LGTM: Modal provides clear translation method selection.The modal UI successfully presents users with a choice between standard copy and provider-based translation. The radio button structure with unique IDs per widget instance prevents conflicts.
lang/en/lang.php (1)
41-43: New translation keys support the modal UI.The added keys
translate_method,standard, andcopy_buttonproperly support the new provider selection modal introduced in this PR.formwidgets/mlricheditor/partials/_mlricheditor.htm (1)
9-9: LGTM: Auto-translate handler properly wired.The
data-auto-translate-handlerattribute correctly integrates the auto-translate functionality into the rich editor widget, consistent with the pattern used in other multilingual form widgets.formwidgets/mltext/partials/_mltext.htm (2)
11-11: LGTM: Auto-translate handler properly added.The
data-auto-translate-handlerattribute correctly integrates auto-translate functionality, consistent with other form widgets.
7-7: Control name change properly implemented and verified.The
data-controlattribute change from"multilingual"to"mltext"is correctly handled in mltext.js. The plugin properly registers at line 70 with$.fn.mlTextand initializes matching controls via the selector$('[data-control="mltext"]')on line 87. The underlying multiLingual plugin dependency is available and correctly utilized.formwidgets/mlmarkdowneditor/partials/_mlmarkdowneditor.htm (1)
9-9: LGTM: Auto-translate handler properly wired.The
data-auto-translate-handlerattribute correctly integrates auto-translate functionality into the markdown editor widget, following the same pattern as the other multilingual form widgets.assets/css/multilingual.css (1)
55-55: Z-index: 99 is appropriate and does not conflict with other elements.Verification shows z-index values exist only within multilingual.css (12, 20, and 99). No competing high z-index values were found in other CSS files, JavaScript, or HTML templates. The loading indicator's z-index: 99 stands alone at the top of the stacking hierarchy and does not conflict with navigation menus, modals, or other UI elements. No changes are needed.
formwidgets/MLTextarea.php (1)
17-17: LGTM! Trait integration looks good.The addition of the
MLAutoTranslatetrait is consistent with the broader PR changes and enables auto-translation capabilities for the textarea widget.formwidgets/MLMarkdownEditor.php (1)
18-18: LGTM! Trait integration looks good.The addition of the
MLAutoTranslatetrait is consistent with other form widgets and enables auto-translation support for the Markdown editor.formwidgets/mltextarea/partials/_mltextarea.htm (2)
11-11: LGTM! Auto-translate handler integration looks good.The addition of the
data-auto-translate-handlerattribute correctly wires up the auto-translation event handling for the textarea widget.
7-7: No breaking change—mltextarea correctly uses its own data-control value.The mltextarea widget has a dedicated JavaScript handler (
formwidgets/mltextarea/assets/js/mltextarea.js) that initializes elements withdata-control="mltextarea". This pattern is consistent across all ML widgets (mltext, mlricheditor, mlrepeater, mlmediafinder, mlnestedform, mlmarkdowneditor, mlblocks)—each has its own data-control value matching its widget name. The only exception is mlurl, which uses the genericdata-control="multilingual"without its own handler. No external code referencesdata-control="multilingual"for mltextarea.Likely an incorrect or invalid review comment.
formwidgets/MLRichEditor.php (1)
18-18: LGTM! Trait integration looks good.The addition of the
MLAutoTranslatetrait enables auto-translation capabilities for the rich editor widget, consistent with other form widgets.formwidgets/MLNestedForm.php (1)
18-18: LGTM! Trait integration looks good.The addition of the
MLAutoTranslatetrait enables auto-translation capabilities for the nested form widget.formwidgets/MLText.php (2)
18-18: LGTM! Trait integration looks good.The addition of the
MLAutoTranslatetrait enables auto-translation capabilities for the text widget.
62-62: LGTM! Asset loading updated.Adding the
mltext.jsasset properly wires up the frontend behavior for auto-translation.formwidgets/MLRepeater.php (1)
21-21: LGTM! Trait integration looks good.The addition of the
MLAutoTranslatetrait enables auto-translation capabilities for the repeater widget.assets/less/multilingual.less (2)
46-47: Selector formatting changes look good.The removal of spaces before the child combinator (
>) tightens the selectors while maintaining the same DOM targeting. These are cosmetic changes that improve consistency.Also applies to: 61-62, 71-72, 76-77, 100-100, 125-126
146-148: LGTM! Loading indicator z-index is appropriate.The z-index value of 99 ensures the loading indicator appears above form controls (z-index: 12-20) while staying below modals, which is the correct stacking order.
formwidgets/MLBlocks.php (1)
21-21: LGTM: Trait addition follows established pattern.The MLAutoTranslate trait is correctly added and used consistently across multiple form widgets in this PR.
traits/MLControl.php (2)
8-8: LGTM: Provider configuration integration.The Config facade import and provider retrieval are appropriate for exposing translation provider options to the UI.
Also applies to: 128-128
82-82: Cosmetic formatting changes.These spacing adjustments in string concatenation and control structures improve consistency but don't affect functionality.
Also applies to: 154-154, 158-160, 194-194, 200-200
formwidgets/mlnestedform/assets/js/mlnestedform.js (1)
80-98: LGTM: Consistent parameter destructuring pattern.The updated signature and payload align with the auto-translation architecture introduced across all ML form widgets in this PR.
formwidgets/mlblocks/assets/js/mlblocks.js (1)
93-111: LGTM: Consistent with auto-translation architecture.The parameter destructuring and payload extension align with the pattern applied across all ML widgets in this PR.
formwidgets/mlricheditor/assets/js/mlricheditor.js (1)
51-51: Proper event lifecycle management.The autoTranslateSuccess event is correctly bound in init and unbound in dispose.
Also applies to: 71-71
formwidgets/mlmarkdowneditor/assets/js/mlmarkdowneditor.js (2)
52-52: LGTM! Proper event lifecycle management.The autoTranslateSuccess event is correctly bound during initialization and unbound during disposal, maintaining proper cleanup.
Also applies to: 68-68
91-96: No issues found; implementation correctly guards against standard provider.The verification confirms that
autoTranslateinmultilingual.jsalready implements the required guard. At line 133-134, the method returns early whenprovider == "standard", preventing any AJAX request. The unconditional call toautoTranslatein the code snippet is safe because the filtering occurs withinautoTranslateitself.formwidgets/mlrepeater/assets/js/mlrepeater.js (1)
83-100: LGTM! Consistent payload structure.The updated onCopyLocale correctly destructures the new payload format and passes currentLocale and provider to the backend handler, aligning with the broader auto-translation architecture.
assets/js/multilingual.js (2)
42-45: LGTM! Clean modal interaction.The modal click handler correctly retrieves the selected locale and triggers the copy workflow.
133-135: Good early return for standard provider.The guard clause at line 133 correctly prevents unnecessary AJAX calls when using the standard copy behavior.
formwidgets/mltextarea/assets/js/mltextarea.js (2)
31-43: LGTM! Well-structured initialization.The plugin properly initializes the multiLingual behavior, binds all necessary events, and performs an initial layout update.
45-59: Excellent cleanup in dispose method.The disposal properly unbinds all event handlers, removes plugin data, and nullifies references to prevent memory leaks.
traits/MLAutoTranslate.php (2)
24-40: LGTM! Clean recursive flattening.The flatten method correctly walks nested arrays and collects whitelisted values in order.
50-68: Handle index overflow gracefully.The expand method uses
$flatValues[$idx] ?? $valuewhich correctly falls back to the original value if the index is out of bounds. This is good defensive programming.tests/unit/traits/MLAutoTranslateTest.php (1)
1-293: No code changes needed in the test file.Verification confirms that guzzlehttp/guzzle is properly declared in composer.json as
"^7.10"in the require section. The test failures are due to the test environment not having dependencies installed (composer install not run), not a code defect. The test file code is correct and follows Laravel HTTP testing patterns appropriately. Ensure the test environment runscomposer installbefore executing tests.providers/DeepLTranslateProvider.php (1)
18-27: No critical issue found. The implementation correctly handles DeepL's locale code requirements.DeepL expects canonical uppercase language tags (e.g., "EN", "EN-GB", "EN-US", "PT-BR"), and the codebase confirms this through test calls using simple two-letter codes ('en', 'es'). The
strtoupper()transformation correctly converts these to DeepL's required format.For regional variants with hyphens (e.g., 'en-US'),
strtoupper('en-US')→ 'EN-US' which matches the canonical form. No evidence of underscore-based locale codes (e.g., 'en_US') exists in the current codebase or test suite, so the implementation is suitable for present usage.
| MultiLingual.prototype.autoTranslate = function(copyFromLocale, provider) { | ||
| var self = this | ||
| if (provider == "standard") { | ||
| return | ||
| } | ||
| var currentLocale = this.activeLocale | ||
| var copyFromValue = this.getLocaleValue(copyFromLocale) | ||
|
|
||
| if (!copyFromValue || copyFromLocale === currentLocale) { | ||
| return | ||
| } | ||
|
|
||
| this.$el | ||
| .addClass('loading-indicator-container size-form-field') | ||
| .loadIndicator() | ||
|
|
||
| this.$el.request(this.options.autoTranslateHandler, { | ||
| data: { | ||
| _copy_from_locale: copyFromLocale, | ||
| _copy_from_value: copyFromValue, | ||
| _current_locale: currentLocale, | ||
| _provider: provider, | ||
| }, | ||
| success: function(data) { | ||
| self.$el.trigger('autoTranslateSuccess.oc.multilingual', [data]) | ||
| self.$el.loadIndicator('hide') | ||
| this.success(data) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify autoTranslateHandler is configured for all widgets.
The autoTranslate method requires this.options.autoTranslateHandler to be set. If a widget doesn't configure this handler, the AJAX request will fail silently.
🏁 Script executed:
#!/bin/bash
# Check which widgets configure autoTranslateHandler
rg -nP 'autoTranslateHandler\s*[:=]' --type=php -g 'formwidgets/**/*.php'Length of output: 0
I need to expand the search scope. The initial PHP-specific search didn't yield results. Let me check where autoTranslateHandler is actually configured across all file types and how the options are initialized.
---
🏁 Script executed:
```shell
#!/bin/bash
# Broader search for autoTranslateHandler across all file types
rg -n 'autoTranslateHandler' --type-list | head -20
rg -n 'autoTranslateHandler' -C3
Length of output: 1857
🏁 Script executed:
#!/bin/bash
# Check how options are initialized in multilingual.js
rg -n 'this\.options' assets/js/multilingual.js -C2Length of output: 850
🏁 Script executed:
#!/bin/bash
# Search for where the options object is set/initialized for the multilingual widget
rg -n 'function.*[Mm]ulti[Ll]ingual|class.*[Mm]ulti[Ll]ingual' --type=js -C5Length of output: 0
Let me examine the full autoTranslate method and surrounding context to see if there's any null checking or error handling:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full autoTranslate method from multilingual.js with more context
cat assets/js/multilingual.js | grep -n . | sed -n '131,165p'
Length of output: 1756
🏁 Script executed:
#!/bin/bash
# Check if there's a null/undefined check before the autoTranslate call
rg -n 'autoTranslate' assets/js/multilingual.js -B5 -A20Length of output: 1853
🏁 Script executed:
#!/bin/bash
# Look for where autoTranslate is invoked and how options are passed
rg -n '\.autoTranslate\(|new.*MultiLingual' --type=php -C5Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where autoTranslate is called in JavaScript
rg -n '\.autoTranslate\(' --type=js -C3
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check backend PHP files for where autoTranslateHandler is set/registered
rg -n 'autoTranslateHandler|auto.*translate' --type=php -C3Length of output: 350
🏁 Script executed:
#!/bin/bash
# Look for the widget registration or where options are passed to MultiLingual
rg -n 'multiLingual|MLText|MLTextarea' --type=php -C2Length of output: 1015
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the MLControl trait
rg -n 'trait MLControl' --type=php -A30
Length of output: 1448
🏁 Script executed:
#!/bin/bash
# Examine MLAutoTranslate trait fully
cat traits/MLAutoTranslate.phpLength of output: 4023
🏁 Script executed:
#!/bin/bash
# Search for where autoTranslateHandler is actually registered or called
rg -n 'autoTranslate|auto_translate' --type=php -C5Length of output: 4202
🏁 Script executed:
#!/bin/bash
# Look for event handlers that might trigger autoTranslate
rg -n 'trigger|on\(' assets/js/multilingual.js -C2Length of output: 3285
---
🏁 Script executed:
```shell
#!/bin/bash
# Check MLText widget JavaScript for autoTranslate usage
cat formwidgets/mltext/assets/js/mltext.js
Length of output: 2794
🏁 Script executed:
#!/bin/bash
# Check MLTextarea widget JavaScript for autoTranslate usage
cat formwidgets/mltextarea/assets/js/mltextarea.jsLength of output: 4590
🏁 Script executed:
#!/bin/bash
# Search all JS files for calls to autoTranslate
rg -n '\.autoTranslate\s*\(' --type=jsLength of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Check MLText PHP widget for autoTranslateHandler configuration
cat formwidgets/MLText.php
Length of output: 1183
🏁 Script executed:
#!/bin/bash
# Check MLTextarea PHP widget for autoTranslateHandler configuration
cat formwidgets/MLTextarea.phpLength of output: 1332
🏁 Script executed:
#!/bin/bash
# Search PHP widgets for makeOptions or where options are set to frontend
rg -n 'makeOptions|prepareVars|getJsonValue|options\[' formwidgets/*.php -A2 -B2Length of output: 2160
---
🏁 Script executed:
```shell
#!/bin/bash
# Search MLControl trait for prepareLocaleVars method
rg -n 'prepareLocaleVars' traits/MLControl.php -A20
Length of output: 818
🏁 Script executed:
#!/bin/bash
# Find view/partial files for these widgets
fd -e 'php' formwidgets/ | grep -i 'view\|partial' | head -20Length of output: 353
🏁 Script executed:
#!/bin/bash
# Check the partial templates for mltext and mltextarea
find formwidgets/mltext formwidgets/mltextarea -name '*.php' 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for where data-auto-translate-handler attribute is set in templates
rg -n 'data-auto-translate-handler' --type=phpLength of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all view/partial files in formwidgets
find formwidgets -type f -name '*.php'
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Look for where widget options are converted to HTML data attributes
rg -n 'makeOptions|data\(' traits/MLControl.php -C3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any reference to autoTranslateHandler in entire codebase
rg -n 'autoTranslateHandler' --type=phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check FormWidgetBase to understand how options become data attributes
rg -n 'getJsonValue' . --type=php -C2 | head -30Length of output: 0
Add autoTranslateHandler configuration to all multilingual widgets.
The autoTranslate method requires this.options.autoTranslateHandler to be set, but this handler is never configured in any form widget. When a non-standard provider is selected, the AJAX request silently fails because null is passed to $.request().
Although the PHP backend defines onAutoTranslate() in the MLAutoTranslate trait and mltextarea.js documents the expected data-auto-translate-handler attribute, no code currently sets this value. Each widget that uses MLAutoTranslate should register the handler in its rendering output (e.g., via data-auto-translate-handler="onAutoTranslate" or similar mechanism in the template).
🤖 Prompt for AI Agents
In assets/js/multilingual.js around lines 131 to 160, autoTranslate calls
this.options.autoTranslateHandler but no widgets set that option, causing
$.request(null) when a non-standard provider is chosen; update widget renderers
that use MLAutoTranslate to output the handler (for example add
data-auto-translate-handler="onAutoTranslate" on the widget root or otherwise
set this.options.autoTranslateHandler during widget initialization), and ensure
the JS reads that attribute into this.options.autoTranslateHandler so the AJAX
request has a valid handler name.
| "php": ">=7.2", | ||
| "winter/wn-backend-module": ">=1.2.8", | ||
| "composer/installers": "~1.0" | ||
| "composer/installers": "~1.0", | ||
| "guzzlehttp/guzzle": "^7.10" |
There was a problem hiding this comment.
❓ Verification inconclusive
Verify PHP version compatibility with Guzzle 7.10.
Guzzle 7.10 requires PHP >=7.2.5, but this package specifies "php": ">=7.2". While this may work, there's a slight mismatch that could cause issues for environments running PHP 7.2.0-7.2.4.
Consider updating the PHP requirement to align with Guzzle's constraint:
- "php": ">=7.2",
+ "php": ">=7.2.5",Alternatively, verify that PHP 7.2.0-7.2.4 support is not needed by running:
🏁 Script executed:
#!/bin/bash
# Check Guzzle 7.10's PHP requirements
curl -s https://repo.packagist.org/p2/guzzlehttp/guzzle.json | jq '.packages["guzzlehttp/guzzle"]["7.10.0"].require.php'Length of output: 65
I'll search for the actual PHP requirements for Guzzle 7.10 to verify the claim in the review comment.
Update PHP requirement to match Guzzle 7.10 dependency.
Guzzle 7.10.0 requires php: ^7.2.5 || ^8.0, but composer.json specifies "php": ">=7.2". Update the PHP requirement to prevent installation issues on PHP 7.2.0-7.2.4:
- "php": ">=7.2",
+ "php": ">=7.2.5",🤖 Prompt for AI Agents
In composer.json around lines 31-34, the "php" constraint is ">=7.2" but Guzzle
7.10 requires "php: ^7.2.5 || ^8.0"; update the "php" requirement to match
Guzzle (for example set "php": "^7.2.5 || ^8.0") to prevent allowing PHP
7.2.0-7.2.4, then run composer validate/composer update to ensure dependency
resolution succeeds.
| /* | ||
| |-------------------------------------------------------------------------- | ||
| | Auto Translation Providers | ||
| |-------------------------------------------------------------------------- | ||
| | | ||
| | Configure the translation API services used by your application. | ||
| | You may define multiple providers and pick one as the default. | ||
| | | ||
| | Supported Examples: "google" | ||
| | | ||
| */ |
There was a problem hiding this comment.
Update supported providers comment.
Line 74 states "Supported Examples: 'google'" but the configuration also supports 'deepl'. Update the comment to reflect both supported providers.
Apply this diff:
- | Supported Examples: "google"
+ | Supported Providers: "google", "deepl"🤖 Prompt for AI Agents
In config/config.php around lines 66 to 76, the comment "Supported Examples:
'google'" is incomplete; update the comment to list both supported translation
providers by changing it to something like "Supported Examples: 'google',
'deepl'". Ensure the wording and punctuation match the surrounding comment
style.
| MLTextarea.prototype.onAutoTranslateSuccess = function(e, data) { | ||
| if (data.translatedValue && data.translatedLocale) { | ||
| var translatedValue = data.translatedValue[0] | ||
| this.$textarea.val(translatedValue).trigger('input') | ||
| this.$el.multiLingual('setLocaleValue', translatedValue, data.translatedLocale) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add array length validation before accessing index.
Similar to other widgets, this code accesses data.translatedValue[0] without verifying the array is non-empty.
Apply this diff:
MLTextarea.prototype.onAutoTranslateSuccess = function(e, data) {
- if (data.translatedValue && data.translatedLocale) {
+ if (data.translatedValue?.length && data.translatedLocale) {
var translatedValue = data.translatedValue[0]
this.$textarea.val(translatedValue).trigger('input')
this.$el.multiLingual('setLocaleValue', translatedValue, data.translatedLocale)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MLTextarea.prototype.onAutoTranslateSuccess = function(e, data) { | |
| if (data.translatedValue && data.translatedLocale) { | |
| var translatedValue = data.translatedValue[0] | |
| this.$textarea.val(translatedValue).trigger('input') | |
| this.$el.multiLingual('setLocaleValue', translatedValue, data.translatedLocale) | |
| } | |
| } | |
| MLTextarea.prototype.onAutoTranslateSuccess = function(e, data) { | |
| if (data.translatedValue?.length && data.translatedLocale) { | |
| var translatedValue = data.translatedValue[0] | |
| this.$textarea.val(translatedValue).trigger('input') | |
| this.$el.multiLingual('setLocaleValue', translatedValue, data.translatedLocale) | |
| } | |
| } |
🤖 Prompt for AI Agents
In formwidgets/mltextarea/assets/js/mltextarea.js around lines 100 to 106, the
code assumes data.translatedValue is an array and directly accesses index 0;
validate that data.translatedValue is an array with length > 0 (e.g.,
Array.isArray(data.translatedValue) && data.translatedValue.length > 0) before
reading [0], and only then set translatedValue/use it to update the textarea and
locale value; otherwise skip the update or handle a fallback (e.g., use
data.translatedValue if it is a non-empty string) and return early to avoid
runtime errors.
| 'translate_method' => 'Translate Method', | ||
| 'standard' => 'Standard', | ||
| 'copy_button' => 'Copy', |
There was a problem hiding this comment.
🧩 Analysis chain
Removed translation key is still referenced in codebase.
The copy_confirm key was removed, but it's still referenced in multiple form widget partials:
formwidgets/mlricheditor/partials/_mlricheditor.htm(line 6)formwidgets/mlmarkdowneditor/partials/_mlmarkdowneditor.htm(line 6)formwidgets/mltext/partials/_mltext.htm(line 8)
This will cause missing translation errors in the UI.
Verify all references to the removed key:
Either restore the translation key or remove all references to it from the codebase.
🏁 Script executed:
#!/bin/bash
# Search for copy_confirm references
rg -n "copy_confirm" --type=phpLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for copy_confirm in all files, focusing on template partials
rg -n "copy_confirm" --type=htm --type=vueLength of output: 27
🏁 Script executed:
#!/bin/bash
# Broader search without type restriction to catch all references
rg -n "copy_confirm"Length of output: 1244
🏁 Script executed:
#!/bin/bash
# Verify the mentioned files exist and check their content
fd -e htm . -x grep -l "copy_confirm" {}Length of output: 453
Removed translation key copy_confirm is still referenced across 9 form widget partials.
The copy_confirm key was removed from lang/en/lang.php, but it's still actively referenced in the following template files:
- formwidgets/mlurl/partials/_mlurl.htm (line 5)
- formwidgets/mltext/partials/_mltext.htm (line 8)
- formwidgets/mlricheditor/partials/_mlricheditor.htm (line 6)
- formwidgets/mltextarea/partials/_mltextarea.htm (line 8)
- formwidgets/mlrepeater/partials/_mlrepeater.htm (line 6)
- formwidgets/mlnestedform/partials/_mlnestedform.htm (line 6)
- formwidgets/mlmarkdowneditor/partials/_mlmarkdowneditor.htm (line 6)
- formwidgets/mlmediafinder/partials/_mlmediafinder.htm (line 5)
- formwidgets/mlblocks/partials/_mlblocks.htm (line 6)
Each references the key via trans('winter.translate::lang.locale.copy_confirm'). This will produce missing translation errors in the UI.
Either restore the translation key to lang/en/lang.php or remove all data-copy-confirm attributes from these template files.
🤖 Prompt for AI Agents
In lang/en/lang.php around lines 41 to 43, the translation key copy_confirm was
removed but is still referenced by nine form widget partials (listed in the
review); restore the missing key by adding a copy_confirm entry to
lang/en/lang.php (e.g. a short confirmation message like "Are you sure you want
to copy?") so trans('winter.translate::lang.locale.copy_confirm') resolves, or
alternatively remove the data-copy-confirm attributes from the nine templates
(formwidgets/mlurl/partials/_mlurl.htm, formwidgets/mltext/partials/_mltext.htm,
formwidgets/mlricheditor/partials/_mlricheditor.htm,
formwidgets/mltextarea/partials/_mltextarea.htm,
formwidgets/mlrepeater/partials/_mlrepeater.htm,
formwidgets/mlnestedform/partials/_mlnestedform.htm,
formwidgets/mlmarkdowneditor/partials/_mlmarkdowneditor.htm,
formwidgets/mlmediafinder/partials/_mlmediafinder.htm,
formwidgets/mlblocks/partials/_mlblocks.htm) to eliminate missing translation
errors.
ProviderFactory.php
Outdated
| return match ($provider) { | ||
| 'google' => new GoogleTranslateProvider($config), | ||
| 'deepl' => new DeepLTranslateProvider($config), | ||
| default => throw new \Exception("No provider found: $provider"), | ||
| }; |
There was a problem hiding this comment.
Replace PHP 8.0 match expression for PHP 7.4 compatibility.
The pipeline failure confirms that the match expression breaks on PHP 7.4. Replace it with a switch statement or if-else chain.
Apply this diff:
- return match ($provider) {
- 'google' => new GoogleTranslateProvider($config),
- 'deepl' => new DeepLTranslateProvider($config),
- default => throw new \Exception("No provider found: $provider"),
- };
+ switch ($provider) {
+ case 'google':
+ return new GoogleTranslateProvider($config);
+ case 'deepl':
+ return new DeepLTranslateProvider($config);
+ default:
+ throw new \Exception("Unknown translation provider: $provider. Supported providers: google, deepl");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return match ($provider) { | |
| 'google' => new GoogleTranslateProvider($config), | |
| 'deepl' => new DeepLTranslateProvider($config), | |
| default => throw new \Exception("No provider found: $provider"), | |
| }; | |
| switch ($provider) { | |
| case 'google': | |
| return new GoogleTranslateProvider($config); | |
| case 'deepl': | |
| return new DeepLTranslateProvider($config); | |
| default: | |
| throw new \Exception("Unknown translation provider: $provider. Supported providers: google, deepl"); | |
| } |
🧰 Tools
🪛 GitHub Actions: Tests
[error] 21-21: Parse error: Unexpected '=>' (T_DOUBLE_ARROW) due to PHP match expression used with PHP 7.4. Upgrade PHP to at least 8.0 or refactor to a compatible switch/if structure.
🤖 Prompt for AI Agents
In ProviderFactory.php around lines 20 to 24, the PHP 8.0 match expression is
not compatible with PHP 7.4; replace it with an equivalent switch (or if/else)
that checks $provider and returns new GoogleTranslateProvider($config) for
'google', new DeepLTranslateProvider($config) for 'deepl', and in the default
case throw a new \Exception("No provider found: $provider"); ensure the method
returns the created provider in every branch and preserves the same exception
message and namespace.
| public function translate(array $input, string $targetLocale, string $currentLocale): array | ||
| { | ||
| $query = http_build_query([ | ||
| 'target' => $targetLocale, | ||
| 'source' => $currentLocale, | ||
| 'key' => $this->config['key'], | ||
| ]); | ||
|
|
||
| foreach ($input as $text) { | ||
| $query .= '&q=' . urlencode($text); | ||
| } | ||
|
|
||
| $endpoint = rtrim($this->config['url'], '/') . '?' . $query; | ||
|
|
||
| $response = Http::get($endpoint); | ||
|
|
||
| if (!$response->successful()) { | ||
| throw new Exception("Google Translation failed: " . $response->body()); | ||
| } | ||
|
|
||
| $json = $response->json(); | ||
|
|
||
| return array_map(fn($t) => | ||
| urldecode(html_entity_decode($t['translatedText'], ENT_QUOTES | ENT_HTML5, 'UTF-8')), | ||
| $json['data']['translations'] | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add HTTP timeout and consider request size limits.
The HTTP request lacks a timeout, which could cause the application to hang on network issues or slow API responses. Additionally, appending all texts to the query string could exceed URL length limits with many or long translation strings.
Apply this diff to add a timeout:
- $response = Http::get($endpoint);
+ $response = Http::timeout(30)->get($endpoint);Consider switching to POST requests for larger translation batches to avoid URL length limitations, as the Google Translate API supports both GET and POST methods.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function translate(array $input, string $targetLocale, string $currentLocale): array | |
| { | |
| $query = http_build_query([ | |
| 'target' => $targetLocale, | |
| 'source' => $currentLocale, | |
| 'key' => $this->config['key'], | |
| ]); | |
| foreach ($input as $text) { | |
| $query .= '&q=' . urlencode($text); | |
| } | |
| $endpoint = rtrim($this->config['url'], '/') . '?' . $query; | |
| $response = Http::get($endpoint); | |
| if (!$response->successful()) { | |
| throw new Exception("Google Translation failed: " . $response->body()); | |
| } | |
| $json = $response->json(); | |
| return array_map(fn($t) => | |
| urldecode(html_entity_decode($t['translatedText'], ENT_QUOTES | ENT_HTML5, 'UTF-8')), | |
| $json['data']['translations'] | |
| ); | |
| } | |
| public function translate(array $input, string $targetLocale, string $currentLocale): array | |
| { | |
| $query = http_build_query([ | |
| 'target' => $targetLocale, | |
| 'source' => $currentLocale, | |
| 'key' => $this->config['key'], | |
| ]); | |
| foreach ($input as $text) { | |
| $query .= '&q=' . urlencode($text); | |
| } | |
| $endpoint = rtrim($this->config['url'], '/') . '?' . $query; | |
| $response = Http::timeout(30)->get($endpoint); | |
| if (!$response->successful()) { | |
| throw new Exception("Google Translation failed: " . $response->body()); | |
| } | |
| $json = $response->json(); | |
| return array_map(fn($t) => | |
| urldecode(html_entity_decode($t['translatedText'], ENT_QUOTES | ENT_HTML5, 'UTF-8')), | |
| $json['data']['translations'] | |
| ); | |
| } |
🤖 Prompt for AI Agents
In providers/GoogleTranslateProvider.php around lines 18 to 44, the HTTP call
lacks a timeout and builds a potentially oversized GET query by appending all
texts to the URL; add a reasonable timeout to the request (e.g.
Http::timeout(10) or configurable) and switch to a POST with a JSON body when
the combined query would be large or when input count/total length exceeds a
safe threshold (or always use POST for simplicity). Build the POST payload with
keys: 'target', 'source', 'key', and 'q' as an array of texts (no URL-encoding),
and call Http::timeout(...)->post($endpoint, $payload); retain the same success
check, JSON parsing and returning of translatedText, and preserve existing error
handling when the response is not successful.
| <div class="modal-header"> | ||
| <button type="button" class="close" data-dismiss="modal" aria-hidden="true">×</button> | ||
| <h4 class="modal-title"></h4> | ||
| </div> |
There was a problem hiding this comment.
Modal title is empty.
The modal header includes a title element, but it's not populated with any text. This creates a confusing user experience.
Consider adding a title:
<div class="modal-header">
<button type="button" class="close" data-dismiss="modal" aria-hidden="true">×</button>
- <h4 class="modal-title"></h4>
+ <h4 class="modal-title"><?= e(trans('winter.translate::lang.locale.copy_from', ['locale' => ''])); ?></h4>Or populate it dynamically via JavaScript when the modal opens.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="modal-header"> | |
| <button type="button" class="close" data-dismiss="modal" aria-hidden="true">×</button> | |
| <h4 class="modal-title"></h4> | |
| </div> | |
| <div class="modal-header"> | |
| <button type="button" class="close" data-dismiss="modal" aria-hidden="true">×</button> | |
| <h4 class="modal-title"><?= e(trans('winter.translate::lang.locale.copy_from', ['locale' => ''])); ?></h4> | |
| </div> |
🤖 Prompt for AI Agents
In traits/mlcontrol/partials/_locale_copy.htm around lines 21 to 24, the modal
header contains an empty <h4 class="modal-title"></h4>; add a meaningful static
title (e.g., "Copy Locale") or wire this element to be populated dynamically
when the modal opens (set its innerText or textContent in the JS that triggers
the modal, and ensure any i18n text uses the correct translation key).
|
I set up the Google Translation API and seems to work at first glance |
Losing.translations.mp4When swapping languages, (not saving in between), the translation disappears and the text is as if the translation never happened but a simple copy instead Does not happen on a text field, there it works as expected? |
|
Translating a repeater |
|
Works; yet does nothing on images |
The popup looks a bit sad 😂
I do override CSS so this might be my bad.
I don't think its up to me to decide how the WinterCMS UI looks like, but maybe this is a better start (adjusted some margins/paddings, moved the NL->FR to the header title and set the container to flex |
|
Since Deepl is not set up (for now) on my end, it shouldn't be in the list |
|
I put If I understand it correctly, only field 'slug' can be translated? Also wondering wether a blacklist wouldn't make more sense than a whitelist |
|
Also wondering what the |
|
@AIC-BV Thanks for all the feedback, I am going to investigate what you brought up here, I do have a few questions though
The expected behaviour on this would be to just do nothing when there is no text to translate right, or was there text to translate? If there was could you please send a screenshot/recording of the steps you took to make this happen?
I didn't wire up the image copy because I figured it doesn't make sense to autotranslate an image path. But it is definitely an oversight that it even gave you the option. But that begs the question, were you expecting some sort of automatic path translation. IMO this might be more of a headache than it's worth, because the api won't know the actual path that you are expecting to get. I will ping you when I have a new commit you can check out, thanks for this! |
|
@IsaiahPaget Completely understand the image argument. An another note, I do have a task open to "auto translate images" but that will require some more work. After this PR we can pick this up, if this is possible and if okay for @LukeTowers |
|
@IsaiahPaget We knew the repeater wasn't going to be easy, see video repeater.auto.translation.mp4Current structure: Since blocks is a repeater, 'blocks' is added to the translatable array blocks.yaml (simplified example) Perhaps the nestedform is the cause? |
|
@AIC-BV For that repeater error, can you paste the full error here? Thanks! |
better errors using, select instead of radio group fix the richeditor Fixed mediafinder issue, updated config, updated the way whitelist is defined
IsaiahPaget
force-pushed
the
wip-auto-translation
branch
from
cf6142e to
00bc81b
Compare
|
@AIC-BV I have addressed the issues that you have brought up here, and updated the config file to have better documentation of the feature, and added errors that are more clear, hope this work, let me know if you have any more issues! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
providers/TranslationProvider.php (1)
5-11: Align interface docblock with actual contract and exceptionsThe PHPDoc currently only documents the
$inputparameter and return type, but the signature also has$targetLocaleand$currentLocale, and implementations throw on API failures.Updating the docblock will make the contract clearer for implementers and callers:
interface TranslationProvider { /** * @param string[] $input - * @return string[] translated output + * @param string $targetLocale Target language code (e.g. "fr") + * @param string $currentLocale Source language code (e.g. "en") + * @return string[] translated output, in the same order as $input + * @throws \Exception When the underlying translation API request fails */ public function translate(array $input, string $targetLocale, string $currentLocale): array; }traits/mlcontrol/partials/_locale_copy.htm (1)
21-35: Fill in the modal title for better UX/accessibilityThe
<h4 class="modal-title"></h4>is still empty, so the dialog opens without a visible heading. This is confusing and not great for accessibility (screen readers rely on it).Consider either:
- Rendering a translated static label here (e.g. “Copy locale” /
winter.translate::lang.locale.copy_from), or- Populating it dynamically in the JS that opens the modal.
providers/GoogleTranslateProvider.php (1)
18-44: Add HTTP timeout and consider POST for larger payloads.The HTTP request lacks a timeout, which could cause the application to hang on network issues. Additionally, building a GET query string could exceed URL length limits with many or long translation strings.
Minimally, add a timeout:
- $response = Http::get($endpoint); + $response = Http::timeout(30)->get($endpoint);For production use, consider switching to POST to avoid URL length limitations, as the Google Translate API supports both methods.
formwidgets/mlmarkdowneditor/assets/js/mlmarkdowneditor.js (1)
98-105: Add array length check before accessing index.Accessing
data.translatedValue[0]without verifying the array is non-empty could result in undefined behavior if the translation returns an empty array.Apply this diff:
MLMarkdownEditor.prototype.onAutoTranslateSuccess = function(e, data) { + if (!data.translatedValue?.length) { + return + } const translatedValue = data.translatedValue[0] - if (typeof translatedValue != 'string' || !this.$markdownEditor.data('oc.markdownEditor')) { + if (typeof translatedValue !== 'string' || !this.$markdownEditor.data('oc.markdownEditor')) { return } this.$el.multiLingual('setLocaleValue', translatedValue, data.translatedLocale) this.$markdownEditor.markdownEditor('setContent', translatedValue); }assets/js/multilingual.js (1)
117-120: Add defensive check for modal existence.If the modal element is missing from the DOM, the selector query will fail silently, potentially causing unexpected behavior.
Apply this diff:
MultiLingual.prototype.getProvider = function() { + if (!this.$modal || !this.$modal.length) { + return 'standard' + } const $select = $('select[name^="translation_provider_"]', this.$modal); return $select.val() ?? 'standard'; }
🧹 Nitpick comments (6)
formwidgets/mlmediafinder/assets/js/mlmediafinder.js (1)
99-101: Consider destructuring only the needed parameter.The
provider,copyFromLocale, andcurrentLocaleparameters are destructured but never used. Since media fields are marked as non-translatable (perautoTranslatable = falsein the PHP widget), the simple copy behavior is correct.If these parameters are present solely for interface consistency with other form widgets, the current approach is fine. Otherwise, you could simplify to:
-MLMediaFinder.prototype.onCopyLocale = function(e, {copyFromLocale, copyFromValue, currentLocale, provider}) { +MLMediaFinder.prototype.onCopyLocale = function(e, {copyFromValue}) { this.setPath(copyFromValue) }assets/less/multilingual.less (1)
7-10: Modal label and locale-heading styles improve clarity; watch global impact of!importantThe
.ml-modal labeland.lang-code-displayrules should make the popup look cleaner (normal‑case labels, strong locale header with spacing). The only caveat is the use of!importanton color/text-transform, which may override backend theme overrides.If you expect custom backend themes, consider scoping more tightly (e.g.
.ml-modal .form-group > label) or dropping!importantif core styles allow it.Also applies to: 15-18
formwidgets/mlnestedform/assets/js/mlnestedform.js (1)
80-99: UpdatedonCopyLocalewiring matches provider-based flow; minor nits onlyThe new handler correctly:
- Accepts the structured event payload
{copyFromLocale, currentLocale, provider}.- Posts
_repeater_copy_locale,_repeater_current_locale, and_providerexpected byMLNestedForm::onCopyItemLocale().- Shows a loading indicator and now hides it in
complete, covering both success and failure paths.Two small nits you might consider (non-blocking):
copyFromValueis destructured but unused; you can drop it from the parameter list unless you plan to use it later.- The
_repeater_*key names are a bit misleading in the context of a nested form widget; if you ever touch this again, renaming to_nestedform_*(and updating the PHP) would be clearer.providers/ProviderFactory.php (1)
12-25: Factory logic is sound; consider minor cleanup and future-proofingThe factory correctly:
- Fails fast when config for a provider is missing.
- Instantiates the expected concrete providers for
'google'and'deepl'.A couple of small follow-ups you might consider (not required for this PR):
- Remove the
breakstatements afterreturnin each case – they’re unreachable.- If you plan to support third‑party providers later, a simple config‑driven class map (e.g.
providers.$provider.class) would let plugin authors register providers without editing this switch.For example, a minimal cleanup could look like:
- switch ($provider) { - case 'google': return new GoogleTranslateProvider($config); break; - case 'deepl': return new DeepLTranslateProvider($config); break; - default: throw new \Exception("No provider found: $provider"); break; - }; + switch ($provider) { + case 'google': + return new GoogleTranslateProvider($config); + case 'deepl': + return new DeepLTranslateProvider($config); + default: + throw new \Exception("No provider found: $provider"); + }assets/js/multilingual.js (1)
31-31: Consider using data attribute for modal selector.The modal is selected using a class (
.ml-modal) while other elements consistently use data attributes. Using a data attribute would be more specific and align with the existing pattern.- this.$modal = $('.ml-modal', this.$el) + this.$modal = $('[data-ml-modal]', this.$el)If you make this change, update the corresponding HTML template to use
data-ml-modalinstead of theml-modalclass for the modal element.traits/MLAutoTranslate.php (1)
10-10: Remove unused import.The
Illuminate\Support\Facades\Httpimport is not used anywhere in this trait. The HTTP calls are made by the provider classes, not by this trait.use Exception; -use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Config;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
assets/css/multilingual.css(2 hunks)assets/js/multilingual.js(4 hunks)assets/less/multilingual.less(7 hunks)config/config.php(1 hunks)formwidgets/MLMediaFinder.php(1 hunks)formwidgets/mlblocks/assets/js/mlblocks.js(2 hunks)formwidgets/mlmarkdowneditor/assets/js/mlmarkdowneditor.js(3 hunks)formwidgets/mlmediafinder/assets/js/mlmediafinder.js(1 hunks)formwidgets/mlnestedform/assets/js/mlnestedform.js(2 hunks)formwidgets/mlrepeater/assets/js/mlrepeater.js(2 hunks)formwidgets/mlricheditor/assets/js/mlricheditor.js(3 hunks)formwidgets/mltext/assets/js/mltext.js(1 hunks)providers/DeepLTranslateProvider.php(1 hunks)providers/GoogleTranslateProvider.php(1 hunks)providers/ProviderFactory.php(1 hunks)providers/TranslationProvider.php(1 hunks)tests/unit/traits/MLAutoTranslateTest.php(1 hunks)traits/MLAutoTranslate.php(1 hunks)traits/MLControl.php(5 hunks)traits/mlcontrol/partials/_locale_copy.htm(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- assets/css/multilingual.css
- traits/MLControl.php
- formwidgets/mltext/assets/js/mltext.js
- tests/unit/traits/MLAutoTranslateTest.php
- config/config.php
- providers/DeepLTranslateProvider.php
🧰 Additional context used
🧬 Code graph analysis (10)
formwidgets/mlmediafinder/assets/js/mlmediafinder.js (1)
formwidgets/MLMediaFinder.php (1)
MLMediaFinder(16-108)
providers/TranslationProvider.php (3)
providers/DeepLTranslateProvider.php (1)
translate(18-41)providers/GoogleTranslateProvider.php (1)
translate(18-44)traits/MLAutoTranslate.php (1)
translate(145-157)
providers/ProviderFactory.php (2)
providers/GoogleTranslateProvider.php (1)
GoogleTranslateProvider(9-45)providers/DeepLTranslateProvider.php (1)
DeepLTranslateProvider(9-42)
formwidgets/mlmarkdowneditor/assets/js/mlmarkdowneditor.js (4)
formwidgets/MLMarkdownEditor.php (1)
MLMarkdownEditor(15-100)assets/js/multilingual.js (1)
provider(166-166)formwidgets/mlricheditor/assets/js/mlricheditor.js (1)
translatedValue(104-104)formwidgets/mltext/assets/js/mltext.js (1)
translatedValue(61-61)
formwidgets/mlblocks/assets/js/mlblocks.js (2)
formwidgets/MLBlocks.php (1)
MLBlocks(18-248)assets/js/multilingual.js (1)
provider(166-166)
providers/GoogleTranslateProvider.php (3)
providers/DeepLTranslateProvider.php (1)
translate(18-41)providers/TranslationProvider.php (1)
translate(11-11)traits/MLAutoTranslate.php (1)
translate(145-157)
formwidgets/mlnestedform/assets/js/mlnestedform.js (2)
formwidgets/MLNestedForm.php (1)
MLNestedForm(15-223)assets/js/multilingual.js (1)
provider(166-166)
traits/MLAutoTranslate.php (4)
providers/ProviderFactory.php (2)
ProviderFactory(10-27)create(12-26)providers/DeepLTranslateProvider.php (1)
translate(18-41)providers/GoogleTranslateProvider.php (1)
translate(18-44)providers/TranslationProvider.php (1)
translate(11-11)
formwidgets/mlricheditor/assets/js/mlricheditor.js (4)
formwidgets/MLRichEditor.php (1)
MLRichEditor(15-108)assets/js/multilingual.js (1)
provider(166-166)formwidgets/mlmarkdowneditor/assets/js/mlmarkdowneditor.js (1)
translatedValue(99-99)formwidgets/mltext/assets/js/mltext.js (1)
translatedValue(61-61)
formwidgets/mlrepeater/assets/js/mlrepeater.js (2)
formwidgets/MLRepeater.php (1)
MLRepeater(18-247)assets/js/multilingual.js (1)
provider(166-166)
🔇 Additional comments (16)
formwidgets/MLMediaFinder.php (1)
65-65: Correctly disables auto-translation for media fields.Setting
autoTranslatable = falseis the right approach for media finder fields, as auto-translating file paths would be semantically incorrect. This aligns with the PR discussion where the author noted that image translation was intentionally not wired up.traits/mlcontrol/partials/_locale_copy.htm (1)
31-42: Provider dropdown + autoTranslatable gating look correct; verify flags for non-text widgetsThe provider
<select>correctly:
- Prefers the configured
$defaultProviderwhen set (via$default/$isStandard), and- Always exposes the “standard” option while only rendering machine providers when
$autoTranslatableis truthy.This should address the earlier concern about images/media offering translation providers that do nothing, as long as those widgets pass
$autoTranslatable = false.Can you double‑check that all non‑machine‑translatable widgets (e.g. MediaFinder/image fields) actually set
$autoTranslatabletofalsewhen including this partial?Also applies to: 44-59
assets/less/multilingual.less (1)
50-52: Dropdown positioning and loading-indicator layering look consistent with new flowThe adjustments to
.dropdown.ml-*.open>.dropdown-menutopoffsets for repeaters, nestedforms, media finder, and editors, plus the.field-multilingual .loading-indicator { z-index: 99; }, should help:
- Keep the copy/locale dropdown aligned with the buttons across widget types.
- Ensure the loading spinner overlays the field correctly during copy/translate.
No functional issues stand out here; just make sure to sanity‑check in the repeater+nestedform case that was failing before, on both light and fancy layouts.
Also applies to: 65-68, 75-78, 80-84, 129-132, 150-152, 157-160
formwidgets/mlricheditor/assets/js/mlricheditor.js (2)
51-51: LGTM: Event lifecycle management is correct.The autoTranslateSuccess event binding and unbinding follow the established pattern and are correctly paired.
Also applies to: 71-71
96-101: LGTM: Signature update aligns with new translation flow.The destructured parameter object and autoTranslate trigger properly integrate with the provider-based translation system.
formwidgets/mlblocks/assets/js/mlblocks.js (1)
93-112: LGTM: Consistent with updated translation flow.The signature change, additional payload fields, and loading indicator management in the complete callback properly support the new provider-based translation system.
formwidgets/mlmarkdowneditor/assets/js/mlmarkdowneditor.js (1)
52-52: LGTM: Event handling and signature update are correct.The autoTranslateSuccess event lifecycle and updated onCopyLocale signature properly integrate with the new translation flow.
Also applies to: 68-68, 91-96
formwidgets/mlrepeater/assets/js/mlrepeater.js (1)
83-102: LGTM: Implementation aligns with translation API updates.The signature change and payload extensions properly support the provider-based translation workflow, with appropriate loading state management.
assets/js/multilingual.js (2)
164-177: LGTM: Event payload structure supports new translation flow.The structured event payload with provider and locale information properly enables the downstream translation workflow.
131-162: Original review comment is incorrect — the autoTranslateHandler is properly configured.The templates correctly set
data-auto-translate-handler="<?= $this->getEventHandler('onAutoTranslate') ?>"for all multilingual widgets (mltext, mltextarea, mlricheditor, mlmarkdowneditor). The multiLingual plugin initialization at line 198 of assets/js/multilingual.js uses$.extend({}, MultiLingual.DEFAULTS, $this.data(), ...), which automatically reads all data attributes from the element. jQuery's.data()method converts the kebab-case attributedata-auto-translate-handlerto camelCase (autoTranslateHandler), properly populatingthis.options.autoTranslateHandler. The corresponding PHP handleronAutoTranslate()is defined in traits/MLAutoTranslate.php. The feature will work as designed.Likely an incorrect or invalid review comment.
traits/MLAutoTranslate.php (6)
24-44: LGTM: Flatten logic correctly handles nested structures.The recursive walk with key normalization (stripping trailing numbers) properly handles repeater and nested form field structures while preserving value order.
54-76: LGTM: Expand logic safely reconstructs the structure.The order-preserving reconstruction with safe array access (using
??operator) properly handles edge cases and maintains structural integrity.
78-94: LGTM: Clean orchestration of translation workflow.The method properly coordinates flattening, translation, and expansion with a helpful error message when no fields are whitelisted.
96-122: LGTM: AJAX handler properly validates and delegates translation.The handler validates required inputs and correctly wraps the single value in an array for the translation provider interface.
123-135: LGTM: Provider configuration resolution is correct.The method properly falls back to the default provider and provides a clear error message when configuration is missing.
145-157: LGTM: Translation delegation is well-structured.The method validates input, resolves the provider configuration, and cleanly delegates to the ProviderFactory and provider implementation.
| MLRichEditor.prototype.onAutoTranslateSuccess = function(e, data) { | ||
| const translatedValue = data.translatedValue[0] | ||
| if (typeof translatedValue != 'string' || !this.$richeditor.data('oc.richEditor')) { | ||
| return | ||
| } | ||
| this.$el.multiLingual('setLocaleValue', translatedValue, data.translatedLocale) | ||
| this.$richeditor.richEditor('setContent', translatedValue); | ||
| } |
There was a problem hiding this comment.
Add array length check before accessing index.
Accessing data.translatedValue[0] without verifying the array is non-empty could result in undefined behavior if the translation returns an empty array.
Apply this diff to add a defensive check:
MLRichEditor.prototype.onAutoTranslateSuccess = function(e, data) {
+ if (!data.translatedValue?.length) {
+ return
+ }
const translatedValue = data.translatedValue[0]
- if (typeof translatedValue != 'string' || !this.$richeditor.data('oc.richEditor')) {
+ if (typeof translatedValue !== 'string' || !this.$richeditor.data('oc.richEditor')) {
return
}
this.$el.multiLingual('setLocaleValue', translatedValue, data.translatedLocale)
this.$richeditor.richEditor('setContent', translatedValue);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MLRichEditor.prototype.onAutoTranslateSuccess = function(e, data) { | |
| const translatedValue = data.translatedValue[0] | |
| if (typeof translatedValue != 'string' || !this.$richeditor.data('oc.richEditor')) { | |
| return | |
| } | |
| this.$el.multiLingual('setLocaleValue', translatedValue, data.translatedLocale) | |
| this.$richeditor.richEditor('setContent', translatedValue); | |
| } | |
| MLRichEditor.prototype.onAutoTranslateSuccess = function(e, data) { | |
| if (!data.translatedValue?.length) { | |
| return | |
| } | |
| const translatedValue = data.translatedValue[0] | |
| if (typeof translatedValue !== 'string' || !this.$richeditor.data('oc.richEditor')) { | |
| return | |
| } | |
| this.$el.multiLingual('setLocaleValue', translatedValue, data.translatedLocale) | |
| this.$richeditor.richEditor('setContent', translatedValue); | |
| } |
🤖 Prompt for AI Agents
In formwidgets/mlricheditor/assets/js/mlricheditor.js around lines 103 to 110,
the code accesses data.translatedValue[0] without ensuring translatedValue is a
non-empty array; add a defensive check such as verifying
Array.isArray(data.translatedValue) && data.translatedValue.length > 0 before
reading index 0, and bail out early if the check fails; then assign
translatedValue = data.translatedValue[0] and keep the existing type and
oc.richEditor checks and subsequent calls unchanged.
| public function __construct(array $config) | ||
| { | ||
| $this->config = $config; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate required configuration keys in constructor.
The constructor accepts configuration without validating that required keys ('url', 'key') are present, leading to unclear errors when they're accessed later.
Apply this diff to add validation:
public function __construct(array $config)
{
+ if (!isset($config['url']) || !isset($config['key'])) {
+ throw new Exception("GoogleTranslateProvider requires 'url' and 'key' configuration");
+ }
$this->config = $config;
}🤖 Prompt for AI Agents
In providers/GoogleTranslateProvider.php around lines 13 to 16, the constructor
currently stores the provided config without verifying required keys, which
causes unclear failures later; update the constructor to check that 'url' and
'key' exist and are non-empty in the input array, and if any are missing throw
an InvalidArgumentException (or similar) with a clear message listing the
missing keys, otherwise assign the validated config to $this->config; ensure
validation runs before any use of the config.
|
Hello @IsaiahPaget sorry for the delay. I tested the feature once more:
I think this feature is nearly complete @LukeTowers what do you think? |
@AIC-BV see https://wintercms.com/docs/v1.2/docs/plugin/settings#file-based-configuration, you would just put your overrides in If you're happy with this PR's functionality as it stands right now @AIC-BV let me know and I'll do a final cleanup pass on the code & UI before I approve it for merging and we send you the invoice. |
@AIC-BV I do think that using "None" would probably be better, and yes the autoTranslateWhitelist is only used on NestedForm, Repeater, and the Blocks widget. @LukeTowers Let me know what you find in code review! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
lang/en/lang.php (1)
40-44: Ensure legacylocale.copy_confirmusages are either removed or the key is restoredYou’ve added the new
translate_method,none, andcopy_buttonkeys, but the olderlocale.copy_confirmkey is no longer present. In earlier revisions, multiple widget partials still referencedwinter.translate::lang.locale.copy_confirm; if any of those remain, this will surface as a missing translation at runtime.To avoid regressions for existing templates and projects:
- Either restore a
copy_confirmentry here (e.g."Are you sure you want to copy?"), or- Remove / migrate all remaining usages of
locale.copy_confirmacross the codebase.#!/bin/bash # Verify no remaining references to the old copy_confirm key rg -n "winter\.translate::lang\.locale\.copy_confirm|locale\.copy_confirm" -Sassets/js/multilingual.js (1)
131-161: EnsureautoTranslateHandleris configured for all multilingual widgets
autoTranslate()calls:this.$el.request(this.options.autoTranslateHandler, { data: { /* ... */ } })If
this.options.autoTranslateHandleris not set for a given widget, this becomes$.request(undefined, …), which will either fail silently or surface as a backend error when a provider is selected.Please double‑check that every widget using provider‑based auto‑translation:
- Outputs a
data-auto-translate-handler(or equivalent) in its markup, or- Otherwise initializes
this.options.autoTranslateHandlerbeforeautoTranslate()can be invoked.traits/mlcontrol/partials/_locale_copy.htm (1)
21-35: Populate the modal title for clearer UXThe modal header still renders an empty
<h4 class="modal-title"></h4>, and there’s no JavaScript inmultilingual.jspopulating it. This leaves the dialog without a visible title, which is confusing and not great for accessibility.Consider either:
- Rendering a static translated title (e.g. “Copy content” or similar), or
- Giving the element a data attribute and populating it dynamically when opening the modal (e.g. “Copy from X to Y”), using an appropriate translation key.
🧹 Nitpick comments (4)
formwidgets/MLRepeater.php (1)
108-123: Guard against missing_providerwhen deciding to auto‑translate
post('_provider')will returnnullif the key is missing; the!== ''check treatsnullas non‑empty and will pass it down toautoTranslateArray(). BecauseMLAutoTranslate::translate()typehints$providerasstring,nullwill be coerced to''and then immediately throw “Cannot translate without a provider”, turning a plain copy into an error.Use an emptiness check so both
nulland''bypass auto‑translation and keep the previous “copy only” behavior:- $provider = post('_provider'); + $provider = post('_provider'); @@ - if ($provider !== '' && !empty($copyFromValues)) { + if (!empty($provider) && !empty($copyFromValues)) { $copyFromValues = $this->autoTranslateArray( $copyFromValues, $currentLocale, $copyFromLocale, $provider ); }traits/MLAutoTranslate.php (3)
9-13: Remove unusedHttpfacade import
MLAutoTranslateno longer callsHttpdirectly; providers and the factory own the HTTP concerns. You can safely drop:use Illuminate\Support\Facades\Http;to keep the trait’s dependencies minimal.
78-85: Clarify whitelist error message to reference field keys, not “labels”When no values are collected from
flatten(), you throw:throw new Exception( "None of the fields on this widget are auto translatable, update config.autoTranslateWhiteList with the label of each input you wish to include, eg. name, content" );But
flatten()compares the base key (e.g.name,content) against the whitelist, not the human‑readable label text. Referring to “label” here can be confusing.Consider rephrasing to something like:
“…update
config.autoTranslateWhiteListwith the field keys you wish to include (for example:name,content).”to match the actual behavior.
96-122: Verify thatonAutoTranslate()returns the expected shape for JS handlers
onAutoTranslate()wraps the result oftranslate()like this:$translated = $this->translate( [$copyFromValue], $currentLocale, $copyFromLocale, $provider ); return [ 'translatedValue' => $translated, 'translatedLocale' => $currentLocale ];Since
translate()always returns an array of strings,translatedValueis an array (e.g.['Hello world']). For simple text widgets, the front‑end often expects a scalar string for the new value.If your
autoTranslateSuccess.oc.multilingualhandlers are treatingtranslatedValueas a scalar, you may want to unwrap it:$translated = $this->translate( [$copyFromValue], $currentLocale, $copyFromLocale, $provider ); return [ 'translatedValue' => $translated[0] ?? $copyFromValue, 'translatedLocale' => $currentLocale, ];Otherwise, confirm that all JS listeners correctly handle
translatedValueas an array.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
assets/js/multilingual.js(4 hunks)config/config.php(1 hunks)formwidgets/MLBlocks.php(2 hunks)formwidgets/MLNestedForm.php(2 hunks)formwidgets/MLRepeater.php(2 hunks)lang/en/lang.php(1 hunks)tests/unit/traits/MLAutoTranslateTest.php(1 hunks)traits/MLAutoTranslate.php(1 hunks)traits/mlcontrol/partials/_locale_copy.htm(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- formwidgets/MLBlocks.php
- formwidgets/MLNestedForm.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/traits/MLAutoTranslateTest.php (1)
traits/MLAutoTranslate.php (3)
translate(145-157)flatten(24-44)expand(54-76)
🪛 GitHub Actions: Tests
tests/unit/traits/MLAutoTranslateTest.php
[error] 25-25: Command 'php artisan winter:test -p Winter.Translate' failed. Error: Class "GuzzleHttp\Psr7\Response" not found.
[error] 49-49: Command 'php artisan winter:test -p Winter.Translate' failed. Error: Class "GuzzleHttp\Psr7\Response" not found.
[error] 83-83: Command 'php artisan winter:test -p Winter.Translate' failed. Error: Class "GuzzleHttp\Psr7\Response" not found.
[error] 106-106: Command 'php artisan winter:test -p Winter.Translate' failed. Error: Class "GuzzleHttp\Psr7\Response" not found.
[error] 143-143: Command 'php artisan winter:test -p Winter.Translate' failed. Error: Class "GuzzleHttp\Psr7\Response" not found.
🔇 Additional comments (3)
config/config.php (1)
66-144: Config structure for auto‑translation looks consistent with trait usageThe new
autoTranslateWhiteList,defaultProvider, andprovidersentries are well‑documented and line up with howMLAutoTranslateandProviderFactoryreadwinter.translate::autoTranslateWhiteList,defaultProvider, andproviders.*. The commented example for adding additional providers should make extension straightforward.tests/unit/traits/MLAutoTranslateTest.php (2)
154-278: Nice coverage offlatten/expandbehavior across real‑world shapesThe
flatten*/expand*tests exercise deep, sparse, and simple structures with the same whitelist, including the trailing‑digit key behavior (does_not_work1, etc.). This gives good confidence that the whitelist semantics match the docs and won’t regress easily.
24-35: Fix HTTP test fakes causingGuzzleHttp\Psr7\ResponseerrorsTests using
Http::fake([...] => Http::response([...], 200))fail withClass "GuzzleHttp\Psr7\Response" not found. This occurs becauseHttp::response()internally instantiates PSR‑7 response objects, which require theguzzlehttp/psr7package.Primary fix:
Addguzzlehttp/psr7(v2.0+ for modern Laravel/Guzzle stacks) as a dev dependency incomposer.json. Since the plugin uses the HTTP client in production, this dependency is likely needed regardless.Alternative approaches:
- Refactor tests to avoid
Http::response()entirely: use simple values likeHttp::fake(['https://fake-endpoint.com/*' => ['data' => [...]]])or string responses.- Stub the translation provider directly instead of mocking HTTP, bypassing the HTTP client layer in tests.
- If a conflicting dependency pins
guzzlehttp/psr7to v1, resolve that version conflict; v2 is required for modern Laravel HTTP testing.Also applies to: 48-59, 82-93, 105-118, 142-147
|
@LukeTowers looks ready to me Thanks @IsaiahPaget for the work |
3 participants
#102
Created a trait that handles translation logic
Updated the copy flow to use a popup to confirm the copy and select provider.
The feature is working, but I would just appreciate some feedback on the UI and maybe the new APIs I made from the perspective of a developer using this plugin.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.