Add image/icon support for FormBuilder::select #222
Conversation
mjauvin
added
the
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Html/FormBuilder.php (2)
485-496: Empty optgroups not rendered in non-legacy modeWhen using the new
itemsformat in non-legacy mode, optgroups with empty item arrays are skipped entirely. The conditionelseif ($items = array_get($display, 'items'))fails for empty arrays since they're falsy, causing the code to fall through and render an<option>instead of an<optgroup>.For example,
'GroupLabel' => ['items' => []]produces an option rather than an empty optgroup.Use
array_key_exists('items', $display)to detect the presence of theitemskey regardless of its value:- if (is_array($display)) { - if ($legacyOptGroup) { - return $this->optionGroup($display, $value, $selected); - } elseif ($items = array_get($display, 'items')) { - return $this->optionGroup($items, $value, $selected); - } - } + if (is_array($display)) { + if ($legacyOptGroup) { + return $this->optionGroup($display, $value, $selected); + } + + if (array_key_exists('items', $display)) { + $items = array_get($display, 'items', []); + return $this->optionGroup($items, $value, $selected); + } + }
515-534: Defensive null checks needed for the array branch inoption()The current implementation can trigger a Deprecation warning in PHP 8.1+ when the array structure is incomplete:
if (is_array($display)) { $data = array_get($display, 1); // ... strpos($data, '.') when $data is null }If
$displayonly has index0(e.g.,['Label']),array_get()returnsnull, andstrpos()will emit a Deprecation warning. In PHP 8.0, this casts to an empty string; in PHP 8.1+, it generates a warning; in PHP 9, it becomes a TypeError.Add type and emptiness checks before calling
strpos():if (is_array($display)) { $label = array_get($display, 0); $data = array_get($display, 1); $display = $label ?? ''; if (is_string($data) && $data !== '') { if (strpos($data, '.') !== false) { $options['data-image'] = $data; } else { $options['data-icon'] = $data; } } }This preserves the
[label, data-attr]API while safely handling missing or unexpected array structures.
🧹 Nitpick comments (1)
src/Html/FormBuilder.php (1)
411-435: NewlegacyOptGroupparameter: good for optgroup control but introduces BC considerations and could be documentedAdding
bool $legacyOptGroup = trueto the publicselect()signature and forwarding it togetSelectOption()is a reasonable way to gate the new behaviour without changing existing call sites. However:
- Any subclasses that override
FormBuilder::select()orFormBuilder::getSelectOption()will now have incompatible signatures in PHP 8+, which can trigger fatal errors at load time.- The new parameter is not mentioned in the docblock for
select(), so its purpose and default behaviour are only discoverable from the signature / implementation.Consider:
- Confirming whether
FormBuilderis commonly subclassed in this ecosystem; if so, call this out in the changelog / upgrade notes.- Optionally updating the
select()docblock to describe$legacyOptGroup(and thattruepreserves legacy behaviour,falseenables treating non‑itemsarrays as regular options).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Html/FormBuilder.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Html/FormBuilder.php (1)
src/Support/helpers-array.php (1)
array_get(187-190)
⏰ 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). (4)
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: windows-latest / PHP 8.2
|
Warning Rate limit exceeded@mjauvin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughSelect rendering now accepts string or array displays; array displays can render single options with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Html/FormBuilder.php(5 hunks)
⏰ 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). (4)
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: windows-latest / PHP 8.3
🔇 Additional comments (2)
src/Html/FormBuilder.php (2)
411-411: LGTM! Backward-compatible signature extension.The new
$legacyOptGroupparameter with a default value oftruemaintains backward compatibility while enabling the new feature.Also applies to: 434-434
485-496: LGTM! Correct conditional logic for new optgroup structure.The method correctly handles both legacy optgroup rendering (when
$legacyOptGroupis true) and the new item-based grouping format (when false). When$legacyOptGroupis false and no 'items' key exists, it appropriately falls through tooption()for image/icon support.
|
@mjauvin in addition to the rabbit's feedback, couldn't we check for the presence of the structure expected by an optiongroup item rather than having a legacyOptGroup parameter? |
|
Also this would need tests. |
LukeTowers
added
the
needs response
I added this so that the previous behavior is maintained, otherwise this would break code using Form::select() with optGroup as just arrays |
|
@mjauvin I don't follow, can you provide code examples? |
I updated the PR description with usage in both modes. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Html/FormBuilder.php (1)
488-491: Preferis_string()for type checking.Using
gettype($keys[0]) === 'string'works correctly butis_string($keys[0])is more idiomatic and readable.Apply this diff:
if (is_array($display)) { $keys = array_keys($display); - if (count($keys) && gettype($keys[0]) === 'string') { + if (count($keys) && is_string($keys[0])) { return $this->optionGroup($display, $value, $selected); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Html/FormBuilder.php(3 hunks)
⏰ 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). (4)
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: windows-latest / PHP 8.2
mjauvin
removed
the
needs response
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/Html/FormBuilderTest.php (4)
344-384: Icon/image option tests are clear and match the intended data- semantics*These two tests cleanly verify the new
['Label', 'icon-or-image']shape for simple, non-grouped options and ensure regular options are unaffected. Behavior and expectations look correct.If you want slightly stronger coverage, consider adding a case where one of these icon/image options is the selected value, to lock in that
selected="selected"anddata-icon/data-imagecoexist as expected.
386-459: Optgroup tests (with and without icons/images) nicely exercise the mixed list shapesThe plain optgroup test and the mixed top-level/optgroup + icon/image test together give good confidence that:
- assoc arrays become
<optgroup>s,- nested
['Label', 'icon-or-image']entries pick updata-icon/data-image,- top-level options and optgroup options interleave correctly.
The assertions look accurate and focused on the key behaviors.
You might add a follow‑up test where a value inside an optgroup that uses the array format (e.g.
group1-opt2) is selected, just to explicitly guard the interaction of selection logic with the new array‑based display format.
524-548: Clarify intent around the “broken” optgroup backward‑compatibility exampleThe test nicely documents the narrow edge case where a 0‑based, two‑item int‑keyed array is now interpreted as
['label', 'iconOrImage']. The expectation for<option value="Group1" data-icon="Option 2">Option 1</option>correctly captures that behavior.To make the test’s purpose even clearer, you could either:
- assert that no option with
value="0"orvalue="1"exists / is selected, or- drop the
selected: 1argument from the call if selection is not part of what you want to demonstrate.That would reduce potential confusion about whether selection behavior is also under test here.
550-572: HTML escaping test is valuable; consider slightly loosening the double‑encoding expectationThis test is important to ensure both option values and labels are escaped and that no raw
<script>/<b>tags leak through. The current assertion onvalue="&lt;script&gt;"tightly couples the test to the exact double‑encoding behavior.If you want the test to be more resilient to future encoding implementation tweaks, you might instead assert that:
- the value contains
<script>(or at leastscript) but not<script>,- and the label contains
<b>Bold Label</b>but not<b>Bold Label</b>,while dropping the strict
&lt;script&gt;check. That still guarantees safety but allows the encoding pipeline to evolve.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Html/FormBuilder.php(4 hunks)tests/Html/FormBuilderTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Html/FormBuilder.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Html/FormBuilderTest.php (1)
src/Html/FormBuilder.php (1)
select(420-455)
⏰ 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). (4)
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: windows-latest / PHP 8.1
🔇 Additional comments (2)
tests/Html/FormBuilderTest.php (2)
461-484: Simple backward‑compatibility test for string options is solidThis test is a good guardrail that:
- plain
key => "Label"options still behave as before (including selection),- no
data-icon/data-imageattributes leak into the legacy shape.Expectations and assertions look correct.
486-522: Optgroup BC test with integer item keys is well targetedThis accurately simulates grouped options coming from a model where item keys are integers, and verifies:
- groups still render as
<optgroup>labels,- integer keys become option values with proper selection,
- no icon/image attributes are introduced.
The scenario and assertions match the documented BC requirement.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Html/FormBuilderTest.php (1)
343-571: Consider adding edge case tests (optional).The test coverage is solid for the main features. To further strengthen reliability, consider adding tests for edge cases:
Malformed array formats:
- Single-element array:
['Label'](missing icon/image)- Three-element array:
['Label', 'icon', 'extra'](unexpected format)Empty/null values:
- Empty icon/image string:
['Label', '']- Null values:
['Label', null]or[null, 'icon']Empty list:
- Behavior when
$listis an empty array[]These edge cases would help document and ensure graceful degradation for unexpected input.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Html/FormBuilderTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Html/FormBuilderTest.php (2)
src/Html/FormBuilder.php (1)
select(420-455)tests/Assertions/AssertHtml.php (2)
assertElementIs(10-19)assertElementAttributeEquals(37-50)
⏰ 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). (4)
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.1
🔇 Additional comments (7)
tests/Html/FormBuilderTest.php (7)
347-363: LGTM! Good coverage for icon data attributes.The test correctly verifies that icon options are rendered with the
data-iconattribute while regular string options remain unchanged.
368-384: LGTM! Good coverage for image data attributes.The test correctly verifies that image options are rendered with the
data-imageattribute.
389-416: LGTM! Solid optgroup test coverage.The test correctly verifies optgroup rendering with multiple groups containing regular options.
421-459: LGTM! Excellent comprehensive test.This test covers the most complex scenario, mixing top-level options (regular and with images) alongside optgroups containing regular options, icons, and images. Great integration test.
464-484: LGTM! Good backward compatibility verification.The test confirms that existing simple string options continue to work without introducing unintended data attributes, and selection behavior is preserved.
489-522: LGTM! Important backward compatibility test for model-based options.This test verifies that optgroups with integer keys (common when using model IDs) continue to work correctly, including selection behavior.
527-547: This is a known, intentional breaking change—not a bug.The detection logic that treats
[0 => 'Option 1', 1 => 'Option 2']as an icon format rather than an optgroup is a documented design trade-off. The implementation prioritizes support for the icon/image syntax['Label', 'icon-name']at the cost of breaking backward compatibility with 0-indexed optgroups containing exactly two items.The test correctly documents this limitation with the docstring "show case where backward compatibility is broken (expected)," and the code comments in
FormBuilder.phpexplain the rationale. Real-world code using 0-indexed arrays for optgroups will break, but this is intentional, not accidental.Recommendation: Ensure this breaking change is prominently documented in user-facing upgrade guides and changelog entries so developers are aware of the migration path.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/Html/FormBuilderTest.php (1)
570-592: Escaping test codifies double‑encoding behavior that was previously flagged as problematic.This test now asserts
value="&lt;script&gt;"for the option whose raw value is<script>, explicitly relying on the value being escaped twice, while the label is only escaped once. A prior review already identified this double encoding path (FormBuilder::option + HtmlBuilder::attributeElement) as likely unintentional; hard‑coding the double‑escaped expectation here will make it harder to fix that bug in the future without changing the test. Consider instead documenting this as current behavior and planning to update both HtmlBuilder and this assertion to a single‑escapedvalue="<script>"if/when the underlying issue is corrected.
🧹 Nitpick comments (1)
tests/Html/FormBuilderTest.php (1)
545-568: Explicit “broken BC” case is useful but could be documented more broadly.The test intentionally locks in the new behavior where a
['Group1' => [0 => 'Option 1', 1 => 'Option 2']]structure is treated as a single option-with-icon, not an optgroup. That’s fine if this edge case is accepted as a deliberate change, but consider also calling this out in user-facing upgrade notes/changelog so consumers relying on 0‑based optgroup keys aren’t surprised.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Html/FormBuilderTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Html/FormBuilderTest.php (2)
src/Html/FormBuilder.php (1)
select(420-455)tests/Assertions/AssertHtml.php (2)
assertElementIs(10-19)assertElementAttributeEquals(37-50)
⏰ 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). (3)
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: windows-latest / PHP 8.4
🔇 Additional comments (7)
tests/Html/FormBuilderTest.php (7)
343-363: Icon select test correctly captures extended option syntax.The expectations for mixing a plain option with an
[label, 'icon-…']entry and assertingdata-iconplus the untouched regular option look correct and exercise the new behavior without over-constraining the markup.
365-384: Image select test cleanly validatesdata-imagebehavior.This mirrors the icon test for image paths and checks both the plain option and the image-bearing option; the assertions look accurate and sufficient.
386-405: Selected image scenario is covered appropriately.Asserting the presence of
selected="selected"alongsidedata-imageon the chosen option (and the untouched regular option) is a good regression check for interaction between selection and the new attribute.
407-437: Baseline optgroup rendering test is solid.This provides a clear control case for simple optgroups (no icons/images), asserting group labels, options, and closing tags, which should help catch regressions in optgroup detection.
439-480: Mixed options + optgroups + icons/images are well covered.This test usefully combines top-level options (including an image) with optgroups containing both plain and icon/image entries, and the assertions line up with the intended rendering semantics.
482-505: Simple backward‑compatibility for string options is clearly specified.Locking in that a flat list of string options still renders identically (including selection and absence of any
data-icon/data-imageattributes) is valuable and the checks are suitably narrow.
507-543: ID‑keyed optgroup backward‑compatibility is tested appropriately.Using integer keys that don’t start at zero to simulate model IDs, and asserting proper
<optgroup>markup with no data attributes, closely matches the BC goal and should protect existing usages.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Html/FormBuilder.php (1)
534-547: Only setdata-icon/data-imagewhen a non‑empty second element is providedThe boolean
strpos()usage is now fixed, butoption()still setsdata-iconordata-imageeven when the second element is missing or empty (e.g.['Label']), resulting in attributes likedata-icon=""and treating any array (even with just one element) as an icon/image pair.To make this robust and match the intended “optional second element” semantics, only add the data attribute when the second element exists and is a non‑empty string, and avoid calling
strpos()on non‑strings:- if (is_array($display)) { - $data = array_get($display, 1, ''); - $display = array_get($display, 0); - if (strpos($data, '.') !== false) { - $options['data-image'] = $data; - } else { - $options['data-icon'] = $data; - } - } + if (is_array($display)) { + $label = array_get($display, 0); + $data = array_get($display, 1); + + if (is_string($data) && $data !== '') { + if (strpos($data, '.') !== false) { + $options['data-image'] = $data; + } else { + $options['data-icon'] = $data; + } + } + + $display = $label; + }This keeps
['Label']working as a simple option label, and only emitsdata-icon/data-imagewhen a real icon/image identifier is passed.
🧹 Nitpick comments (1)
src/Html/FormBuilder.php (1)
524-530: Widenedoption()parameter type may break subclasses overriding the methodChanging
option()toprotected function option(string|array $display, ...)is correct for the new feature, but it can be a BC break for any subclasses that previously overrodeoption(string $display, ...)with a narrower parameter type (PHP will now see them as incompatible with the parent signature).If you expect
FormBuilderto be extended, consider either dropping the concrete parameter type in the method signature (and relying on the phpdoc) or at least documenting this as a potential BC break in the changelog/release notes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Html/FormBuilder.php(3 hunks)tests/Html/FormBuilderTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Html/FormBuilderTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Html/FormBuilder.php (1)
src/Support/helpers-array.php (1)
array_get(187-190)
⏰ 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). (4)
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.2
🔇 Additional comments (2)
src/Html/FormBuilder.php (2)
410-419: select() phpdoc clearly documents the new option formatsThe extended documentation for
$list(simple options, icon/image pairs, optgroups, and mixed formats) is clear and matches the implemented behavior and PR description.
493-507: Current logic correctly routes optgroup and single-option arraysThe current implementation using
array_keys($display) !== [0,1]is logically correct. When this condition is true (keys are not [0,1]), it callsoptionGroup(). Arrays like['Group' => ['Option 1', 'Option 2']]have outer keys of['Group'], not[0,1], so the condition evaluates to true and correctly routes tooptionGroup(). Two-element numeric-keyed arrays like['Label', 'icon.png']have keys of[0,1], so the condition is false and they correctly route tooption().The proposed refactoring, while more explicit in intent, is unnecessary as the current logic already handles all cases correctly.
Enhanced usage:
Backward compatibility is maintained, and once I update the
_field_dropdownpartial for the backend form widget to use the FormBuilder, we'll have support for optGroup in our dropdowns.Example result with mixed formats:
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
Compatibility
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.