Skip to content

Conversation

@mjauvin
Copy link
Member

@mjauvin mjauvin commented Dec 5, 2025

This prevent the node to end-up with an unnecessary "emptyOption" attribute. Summary by CodeRabbit Bug Fixes Fixed an issue where empty option values were incorrectly appearing as attributes in select form elements; dropdowns now render correctly when an empty/placeholder option is configured. Tests Added a test verifying select rendering with an empty/placeholder option and ensuring no stray attributes are produced. ✏️ Tip: You can customize this high-level summary in your review settings.

@mjauvin mjauvin requested a review from LukeTowers December 5, 2025 16:36
@mjauvin mjauvin self-assigned this Dec 5, 2025
@mjauvin mjauvin added the maintenance PRs that fix bugs, are translation changes or make only minor changes label Dec 5, 2025
@coderabbitai
Copy link

coderabbitai bot commented Dec 5, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

The FormBuilder::select method now unsets the emptyOption key after prepending the empty option so it does not appear as an HTML attribute. A new unit test was added to verify the empty option is rendered and that the emptyOption attribute is not present in the final markup.

Changes

Cohort / File(s) Summary
Select Element Empty Option Handling
src/Html/FormBuilder.php
Unset $options['emptyOption'] after prepending the empty option to $list, preventing emptyOption from being rendered as an attribute on the <select> element.
Tests - Select with Empty Option
tests/Html/FormBuilderTest.php
Added testSelectWithEmptyOption() to assert the placeholder empty option is rendered, subsequent options appear, and the emptyOption configuration key is not present in the output attributes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Specific areas for attention:
    • Confirm unset($options['emptyOption']) occurs before $options is used to build HTML attributes.
    • Ensure test covers edge cases (e.g., emptyOption falsy values, pre-existing options list).

Poem

🐰 I nudge the list, an empty leaf in view,
I pull the key that shouldn't walk through,
With one small unset the markup's bright,
Clean options now—hop, tidy, right! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly describes the main change: removing the emptyOption after it has been used to populate the select element's options array.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 92aeecf and 2388514.

📒 Files selected for processing (1)
  • tests/Html/FormBuilderTest.php (1 hunks)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@LukeTowers LukeTowers added this to the 1.2.10 milestone Dec 6, 2025
@LukeTowers LukeTowers merged commit 3de97f6 into develop Dec 6, 2025
10 of 11 checks passed
@LukeTowers LukeTowers deleted the empty-option branch December 6, 2025 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance PRs that fix bugs, are translation changes or make only minor changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants