-
-
Notifications
You must be signed in to change notification settings - Fork 35
Add image/icon support for FormBuilder::select #222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.