Conversation
mjauvin
added
enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new FieldSet backend form widget (class, partial, CSS/LESS, asset registration) and updates backend form save logic to collect values from nested widgets that expose getFormFields(). Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BackendForm as Backend Form
participant FieldSet as FieldSet Widget
participant EmbeddedForm as Embedded Form Widget
participant FormSave as Form.getSaveData()
participant Model
User->>BackendForm: open form (render)
BackendForm->>FieldSet: init()/prepareVars()
FieldSet->>EmbeddedForm: create & bind Form widget (model/data/alias)
EmbeddedForm-->>FieldSet: provide fields & render output
FieldSet-->>BackendForm: render() (includes EmbeddedForm)
User->>BackendForm: submit form
BackendForm->>FormSave: getSaveData()
FormSave->>FieldSet: detect widget with getFormFields()
FieldSet-->>FormSave: expose nested field definitions
FormSave->>FormSave: extract nested values, convert types, merge payload
FormSave->>Model: persist merged save data
Model-->>User: save result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Fix all issues with AI agents
In @modules/backend/formwidgets/fieldset/assets/less/fieldset.less:
- Line 3: The LESS variable declaration for @panel-bg has a double semicolon
("@panel-bg: #f5f5f5;;"); remove the extra semicolon so the declaration is a
single semicolon terminated statement (update the @panel-bg variable in
fieldset.less).
🧹 Nitpick comments (4)
modules/backend/widgets/Form.php (2)
1224-1239: Consider variable naming to avoid shadowing.The inner foreach loop reuses the variable name
$field(line 1226), which shadows the outer loop variable$fieldfrom line 1216. While this doesn't cause functional issues due to scoping, it can reduce code clarity.♻️ Suggested refactor
// get nested widget fields that should be saved to the model if (method_exists($widget, 'getFormFields')) { - foreach ($widget->getFormFields() as $field) { - $parts = HtmlHelper::nameToArray($field->fieldName); - if (($value = $this->dataArrayGet($data, $parts)) !== null) { + foreach ($widget->getFormFields() as $nestedField) { + $parts = HtmlHelper::nameToArray($nestedField->fieldName); + if (($value = $this->dataArrayGet($data, $parts)) !== null) { /* * Number fields should be converted to integers */ - if ($field->type === 'number') { + if ($nestedField->type === 'number') { $value = !strlen(trim($value)) ? null : (float) $value; } $this->dataArraySet($result, $parts, $value); } } }
1228-1237: Consider extracting field value processing logic.The field value processing logic (lines 1228-1237) duplicates the same logic from lines 1201-1210. While the duplication is minimal and the context differs slightly, extracting this into a helper method could improve maintainability.
♻️ Optional refactor to reduce duplication
Consider adding a protected helper method:
protected function processFieldSaveValue($field, $data, &$result) { $parts = HtmlHelper::nameToArray($field->fieldName); if (($value = $this->dataArrayGet($data, $parts)) !== null) { /* * Number fields should be converted to integers */ if ($field->type === 'number') { $value = !strlen(trim($value)) ? null : (float) $value; } $this->dataArraySet($result, $parts, $value); return true; } return false; }Then both locations could use:
foreach ($this->allFields as $field) { if ($field->disabled || $field->hidden) { continue; } $this->processFieldSaveValue($field, $data, $result); } // ...and later... if (method_exists($widget, 'getFormFields')) { foreach ($widget->getFormFields() as $nestedField) { $this->processFieldSaveValue($nestedField, $data, $result); } }modules/backend/formwidgets/fieldset/assets/css/fieldset.css (1)
1-9: Note: Recompile after fixing LESS source.This appears to be the compiled CSS output from the LESS file. After fixing the double semicolon syntax error in
fieldset.less(line 3), remember to recompile this CSS file to ensure consistency.modules/backend/formwidgets/FieldSet.php (1)
21-29: Consider adding type hints to properties.Adding type declarations would improve IDE support and code clarity.
📝 Suggested type hints
/** * @var array Form configuration */ -public $form; +public array $form; /** * @var Form form widget reference */ -protected $formWidget; +protected Form $formWidget;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
modules/backend/ServiceProvider.phpmodules/backend/formwidgets/FieldSet.phpmodules/backend/formwidgets/fieldset/assets/css/fieldset.cssmodules/backend/formwidgets/fieldset/assets/less/fieldset.lessmodules/backend/formwidgets/fieldset/partials/_fieldset.phpmodules/backend/widgets/Form.php
🧰 Additional context used
🧬 Code graph analysis (3)
modules/backend/formwidgets/fieldset/partials/_fieldset.php (1)
modules/backend/formwidgets/FieldSet.php (1)
render(70-74)
modules/backend/widgets/Form.php (1)
modules/backend/formwidgets/FieldSet.php (1)
getFormFields(62-65)
modules/backend/ServiceProvider.php (2)
modules/system/classes/CombineAssets.php (1)
registerBundle(660-697)modules/backend/formwidgets/FieldSet.php (1)
FieldSet(14-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: ubuntu-latest / PHP 8.4
- GitHub Check: ubuntu-latest / PHP 8.2
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: ubuntu-latest / PHP 8.1
- GitHub Check: ubuntu-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: windows-latest / JavaScript
🔇 Additional comments (5)
modules/backend/formwidgets/fieldset/assets/less/fieldset.less (1)
1-9: LGTM!The partial cleanly renders the fieldset with optional translated legend and embedded form widget. The implementation is straightforward and follows Winter CMS conventions.
modules/backend/ServiceProvider.php (1)
100-100: LGTM!Both registrations follow the existing patterns correctly:
- The asset bundle is properly registered for the FieldSet widget
- The form widget registration maintains alphabetical ordering and follows the established convention
Also applies to: 252-252
modules/backend/formwidgets/FieldSet.php (3)
57-65: LGTM!The
loadAssets()andgetFormFields()methods are correctly implemented. ThegetFormFields()method provides the necessary integration point for the Form widget'sgetSaveData()enhancement mentioned in the PR.
67-79: LGTM!The
render()andprepareVars()methods follow standard form widget patterns. Syncing preview mode inprepareVars()ensures the embedded form respects the current state even if it changes after initialization.
44-46: Verify whethergetLoadValue()is appropriate for the parent-level fieldset data structure.The pattern of setting
$config->data = $this->getLoadValue()follows the same approach as NestedForm.php, but since FieldSet saves fields at the parent level (using parent form's arrayName), the data structure implications differ. It's unclear whethergetLoadValue()returns a compatible structure for fields saved at the parent level rather than nested. This needs verification against actual form value handling.Additionally, line 49 directly accesses
$this->getParentForm()->arrayNamewithout null safety, unlike the defensive pattern used in DataTable.php (which checksisset($this->getParentForm()->arrayName)). While NestedForm similarly accesses getParentForm() unsafely, consider adding a null check for consistency and robustness.
|
@mjauvin How is the responsive design being managed? |
It's pretty much derived from a nestedform ... so it renders a form within the The main difference is that fields are exported to the parent when saving (does not use a JSON field). |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @modules/backend/formwidgets/FieldSet.php:
- Line 26: Add a PHPDoc docblock above the public property $showLabels on the
FieldSet class that explains its purpose, type and default value (bool, defaults
to false), describes what enabling it changes (e.g., whether child field labels
within the fieldset are displayed), and any usage notes or expected values;
reference the property name $showLabels and ensure the docblock follows PSR-5
style so IDEs and static analyzers can pick up the type and description.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/backend/formwidgets/FieldSet.phpmodules/backend/formwidgets/fieldset/partials/_fieldset.php
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/backend/formwidgets/fieldset/partials/_fieldset.php
🧰 Additional context used
🧬 Code graph analysis (1)
modules/backend/formwidgets/FieldSet.php (3)
modules/backend/widgets/Form.php (7)
Form(23-1496)init(127-143)bindToController(152-156)loadAssets(161-164)getFields(1032-1035)render(181-229)prepareVars(303-311)modules/system/traits/AssetMaker.php (1)
addCss(157-176)modules/system/traits/ViewMaker.php (1)
makePartial(100-119)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: ubuntu-latest / PHP 8.1
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: ubuntu-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: ubuntu-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: ubuntu-latest / PHP 8.2
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / JavaScript
🔇 Additional comments (3)
modules/backend/formwidgets/FieldSet.php (3)
48-48: Verify data initialization for the nested form.Setting
$config->data = $this->getLoadValue()passes the fieldset field's stored value to the nested Form widget. Since fieldset fields are saved at the parent level (not nested), the fieldset itself shouldn't have a stored value. The nested Form should load field values from the model (set on line 47), not from any value stored under the fieldset's name.If
getLoadValue()returns non-null data (e.g., legacy data or misconfiguration), the nested Form might use incorrect values instead of loading from the model.Consider removing or setting to null:
- $config->data = $this->getLoadValue(); + $config->data = null;Or verify that the Form widget correctly prioritizes model values over the data config when both are provided.
64-67: LGTM!This delegation method correctly exposes the nested form's fields to the parent form's save process, enabling the fields inside the fieldset to be saved at the parent level.
51-51: No action required.The
getParentForm()method is defined inFormWidgetBase(line 99) as a public method, andFieldSetextendsFormWidgetBase, making the method available for use at line 51.
|
@mjauvin can you make a PR to the docs and include examples in the nestedform and fieldset sections as to when you should pick one vs the other? |
Done in wintercms/docs#258 |
| $this->dataArraySet($result, $parts, $value); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Does this work with formwidgets that are embedded in the fieldset's form? Do they get added to the $this->formWidgets of the parent form or should we refactor this a bit so that it's a separate method / inline function that can be called again on every fieldset form inside of the the main form recursively?
Co-authored-by: Luke Towers <git@luketowers.ca>
3 participants
This formwidget wraps form elements with a
<fieldset>HTML tag.The fields are visually grouped but saved as if they were regular fields at the parent level.
The field's label config is used for the
<fieldset>legend.The Form widget's
getSaveData()method now checks for agetFormFields()method on widgets in order to pull child fields from the widget.The field config is akin to a nestedform:
Summary by CodeRabbit
New Features
Style
Bug Fixes