Repository navigation
Conversation
The nested-form guard exists because a repeater's or nested form's fields belong to their own data scope and do not map to the model's translatable attributes. The fieldset widget is different: it reuses the parent form's model and arrayName and merges its save data straight back up, so its fields are model attributes exactly as if they had been declared on the parent form. Core flags that case with Form::$sharesParentScope; the property_exists check keeps this working on core releases that predate it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 51 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change enables translation widgets for fields inside fieldsets while preserving existing nested-form and repeater behavior; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
A form widget stub declaring sharesParentScope itself keeps the new branch covered on core releases that predate the property; the end-to-end case, going through the real fieldset form widget, is skipped until core provides it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The core flag now answers whether a nested form's fields resolve on the model, which a fieldset only does when the form it sits in does too. A fieldset inside a repeater item holds that item's data, so a sub-field sharing its name with a translatable attribute must keep its plain widget. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@AIC-BV can you document |
Explain why fields in nested form widgets are skipped, why the fieldset widget is the exception through Form::$sharesModelScope, and how a custom form widget that reuses the parent form's model scope can opt in. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@mjauvin done in f5d60d9: a new "Fields inside nested form widgets" section under Model translation in the README covers why nested widgets are skipped, why |
|
@AIC-BV please fix Code Quality failing test |
|
@mjauvin not related to the code changes but okay 😀 |
Pre-existing violations that the new Code Quality workflow reports on the changed files: brace placement of an empty catch block and missing spaces after the function keyword on three closures. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Fields nested inside a
fieldsetform widget never get theirml*widget, so translatable attributes grouped in a fieldset are not translatable in the backend.registerModelTranslation()skips any nested form. That guard is right forrepeaterandnestedform: their fields belong to their own data scope and do not map to the model's translatable attributes.fieldsetis different — it reuses the parent form's model andarrayName, and merges itsgetSaveData()straight back up, so its fields are model attributes exactly as if they had been declared on the parent form.Core distinguishes the two cases with a new
Form::$sharesModelScopeflag (wintercms/winter#1528); this checks it, and theproperty_exists()guard keeps the plugin working unchanged on core releases that predate the property.Depends on wintercms/winter#1528. Safe to merge independently: without core support
property_exists()isfalseand behaviour is identical to today. If that PR settles on a different property name, this needs the same rename.Tests
Added to
tests/unit/classes/EventRegistryTest.php:develop(what CI runs)FieldSetwidget)The second test uses a
Formsubclass that declaressharesModelScopeitself, so the new branch stays covered on core releases that predate the property — it does not silently become a no-op on today's CI. The last two go through the real fieldset widget and skip until core provides the flag. Reverting the guard fails tests 2 and 3; hardcoding the core flag totrueinstead of deriving it fails test 4, where a repeater sub-field sharing a name with a translatable attribute would otherwise be turned into anmltext.Full plugin suite green against a patched core: 56 tests, 168 assertions.
Manual verification
Verified on a settings model with 22 translatable attributes, all inside
type: fieldsetgroups: with the change all 22 render asml*widgets inside the fieldset; with the original$widget->isNestedguard restored, 0 — confirming the fieldset's inner form still reportsisNested === true, so nothing else that relies on that flag is affected.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests