fix(form): Remove BaseChoice class and related mixins; update InputCheckbox, InputFile, and InputRadio to extend BaseInput.#56
Conversation
…tCheckbox`, `InputFile`, and `InputRadio` to extend `BaseInput`.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR removes the BaseChoice class and two form traits (CanBeEnclosedByLabel, HasUnchecked), refactors InputCheckbox/InputRadio to extend BaseInput with explicit traits, removes unchecked handling from InputFile, and deletes related tests and adds a changelog entry. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
===========================================
Coverage 100.00% 100.00%
+ Complexity 171 164 -7
===========================================
Files 88 85 -3
Lines 410 345 -65
===========================================
- Hits 410 345 -65 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/Form/InputRadioTest.php (2)
912-921: 🧹 Nitpick | 🔵 TrivialSame reduced immutability coverage as
InputCheckboxTest.Only
checked(true)is tested for immutability. See the comment onInputCheckboxTestfor the broader recommendation — if trait-level immutability is not tested elsewhere, consider adding assertions forautofocus(),required(),tabIndex(), andvalue().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Form/InputRadioTest.php` around lines 912 - 921, The test testReturnNewInstanceWhenSettingAttribute in InputRadioTest only asserts immutability for checked(true); extend it to also assert that calling InputRadio::tag()->autofocus(true), ->required(true), ->tabIndex(0) and ->value('x') each return a new instance (not the same object) to match trait-level immutability coverage—locate the InputRadio class and add assertions mirroring the existing checked(true) assertNotSame for the methods autofocus, required, tabIndex and value.
22-31:⚠️ Potential issue | 🟡 MinorPHPDoc still references label configurations, but label tests were removed.
Same issue as in
InputCheckboxTest— line 30 mentions "Renders label configurations, including content, attributes, and tag name" but those tests no longer exist. Update the docblock.📝 Suggested fix
* Test coverage. * - Applies global and custom attributes, including `aria-*`, `data-*`, `on*` and enum-backed values. * - Applies input radio specific attributes (`autofocus`, `checked`, `disabled`, `form`, `name`, `required`, * `tabindex`, `value`) and renders expected output. * - Handles edge cases for `checked` attribute with various data types and value comparisons. * - Renders attributes and string casting for a void element. - * - Renders label configurations, including content, attributes, and tag name. * - Resolves default and theme providers, including global defaults and user overrides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Form/InputRadioTest.php` around lines 22 - 31, Update the PHPDoc block for the InputRadioTest class to remove the outdated mention of label tests: locate the docblock in InputRadioTest (the comment summarizing test coverage) and delete or replace the line "Renders label configurations, including content, attributes, and tag name" so the summary only reflects the actual tests (global/custom attributes, input-specific attributes, checked edge cases, void element rendering, and provider resolution). Ensure the docblock remains accurate and concise about the existing test cases.tests/Form/InputCheckboxTest.php (1)
22-29:⚠️ Potential issue | 🟡 MinorPHPDoc still references label configurations, but label tests were removed.
Line 28 mentions "Renders label configurations, including content, attributes, and tag name" in the test coverage summary, but all label-related tests have been removed in this PR since
CanBeEnclosedByLabelis gone. The docblock should be updated to reflect the actual test coverage.📝 Suggested fix
* Test coverage. * - Applies global and custom attributes, including `aria-*`, `data-*`, `on*` and enum-backed values. * - Applies input checkbox specific attributes (`autofocus`, `checked`, `disabled`, `form`, `name`, `required`, * `tabindex`, `value`) and renders expected output. * - Handles edge cases for `checked` attribute with various data types and value comparisons. * - Renders attributes and string casting for a void element. - * - Renders label configurations, including content, attributes, and tag name. * - Resolves default and theme providers, including global defaults and user overrides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Form/InputCheckboxTest.php` around lines 22 - 29, Update the PHPDoc block in InputCheckboxTest.php to remove or revise the sentence "Renders label configurations, including content, attributes, and tag name" so the test coverage summary no longer claims label-related tests; open the docblock at the top of the InputCheckboxTest class and either delete that bullet or replace it with an accurate description of current coverage (e.g., note void element rendering or other existing cases) so the comment matches the actual tests after CanBeEnclosedByLabel was removed.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mdsrc/Form/Base/BaseChoice.phpsrc/Form/InputCheckbox.phpsrc/Form/InputFile.phpsrc/Form/InputRadio.phpsrc/Form/Mixin/CanBeEnclosedByLabel.phpsrc/Form/Mixin/HasUnchecked.phptests/Form/InputCheckboxTest.phptests/Form/InputFileTest.phptests/Form/InputRadioTest.php
💤 Files with no reviewable changes (3)
- src/Form/Mixin/HasUnchecked.php
- src/Form/Mixin/CanBeEnclosedByLabel.php
- src/Form/Base/BaseChoice.php
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: terabytesoftw
Repo: ui-awesome/html PR: 35
File: tests/Provider/Form/CheckedProvider.php:13-14
Timestamp: 2026-02-09T11:28:09.334Z
Learning: In the ui-awesome/html repository, using `str_replace` to adapt shared data provider HTML strings for different input types (e.g., adapting `CheckedProvider` for both `InputCheckboxTest` and `InputRadioTest` by replacing "inputcheckbox-" with "inputradio-" and "checkbox" with "radio") is acceptable and considered clear. Don't suggest refactoring such patterns to avoid string replacement.
📚 Learning: 2026-02-09T11:28:09.334Z
Learnt from: terabytesoftw
Repo: ui-awesome/html PR: 35
File: tests/Provider/Form/CheckedProvider.php:13-14
Timestamp: 2026-02-09T11:28:09.334Z
Learning: In the ui-awesome/html repository, using `str_replace` to adapt shared data provider HTML strings for different input types (e.g., adapting `CheckedProvider` for both `InputCheckboxTest` and `InputRadioTest` by replacing "inputcheckbox-" with "inputradio-" and "checkbox" with "radio") is acceptable and considered clear. Don't suggest refactoring such patterns to avoid string replacement.
Applied to files:
tests/Form/InputCheckboxTest.php
📚 Learning: 2026-02-06T21:37:44.509Z
Learnt from: terabytesoftw
Repo: ui-awesome/html PR: 24
File: tests/Form/InputTextTest.php:33-620
Timestamp: 2026-02-06T21:37:44.509Z
Learning: In the ui-awesome/html repository, prefer individual test methods over PHPUnit data providers in test classes. This helps maintainers see exactly how each test behaves without cross-referencing data providers. Apply to all PHP test files under tests. Use separate, descriptively named test methods to cover different inputs/edge cases; retain data providers only if they clearly improve readability or reduce duplication.
Applied to files:
tests/Form/InputCheckboxTest.phptests/Form/InputFileTest.phptests/Form/InputRadioTest.php
📚 Learning: 2026-02-11T14:56:18.277Z
Learnt from: terabytesoftw
Repo: ui-awesome/html PR: 37
File: tests/Form/InputTextTest.php:448-457
Timestamp: 2026-02-11T14:56:18.277Z
Learning: In PHP test files under the repository ui-awesome/html (e.g., tests/Form/InputTextTest.php), it is acceptable to use short variable names (e.g., 2-character names like $id). PHPMD's short-variable rule does not need to be strictly followed in test code. During reviews, allow short names in test files and note that this relaxed style applies to tests, while production code should adhere to the standard variable naming conventions.
Applied to files:
tests/Form/InputCheckboxTest.phptests/Form/InputFileTest.phptests/Form/InputRadioTest.php
📚 Learning: 2026-02-21T01:17:40.877Z
Learnt from: terabytesoftw
Repo: ui-awesome/html PR: 51
File: tests/List/UlTest.php:282-283
Timestamp: 2026-02-21T01:17:40.877Z
Learning: In the ui-awesome/html repository, for PHP tests, ensure that assertion messages in tests/List/UlTest.php for methods named testRenderWith*UsingEnum remain identical to the corresponding non-enum tests (i.e., do not append 'using enum' to the failure message). The reason: the underlying render method treats enum and string inputs the same, so failure output should be uniform. Apply this pattern to all similar test cases under tests/ to maintain consistent failure messages across enum vs non-enum variants.
Applied to files:
tests/Form/InputCheckboxTest.phptests/Form/InputFileTest.phptests/Form/InputRadioTest.php
🧬 Code graph analysis (2)
src/Form/InputFile.php (2)
src/Form/InputCheckbox.php (1)
loadDefault(72-75)src/Form/InputRadio.php (1)
loadDefault(72-75)
src/Form/InputCheckbox.php (2)
src/Form/InputRadio.php (3)
getAttributes(50-53)loadDefault(72-75)run(82-85)src/Form/Mixin/HasCheckedState.php (1)
buildAttributes(74-113)
⏰ 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). (12)
- GitHub Check: phpunit / PHP 8.3-windows-2022
- GitHub Check: linter / Super Linter
- GitHub Check: phpunit / PHP 8.4-windows-2022
- GitHub Check: phpunit / PHP 8.1-windows-2022
- GitHub Check: phpunit / PHP 8.3-ubuntu-latest
- GitHub Check: phpunit / PHP 8.2-windows-2022
- GitHub Check: phpunit / PHP 8.4-ubuntu-latest
- GitHub Check: phpunit / PHP 8.5-ubuntu-latest
- GitHub Check: phpunit / PHP 8.1-ubuntu-latest
- GitHub Check: mutation / PHP 8.5-ubuntu-latest
- GitHub Check: phpunit / PHP 8.4-ubuntu-latest
- GitHub Check: linter / Super Linter
🔇 Additional comments (10)
CHANGELOG.md (1)
43-43: LGTM!Changelog entry is consistent with the PR objectives and follows the established format.
src/Form/InputCheckbox.php (1)
35-85: Clean migration fromBaseChoicetoBaseInput.The implementation correctly composes the required traits and delegates to
buildAttributes()fromHasCheckedStatefor checked-state logic. The structure mirrorsInputRadioexactly (differing only inType::CHECKBOXvsType::RADIO), which is expected given the removal of the sharedBaseChoicebase class.src/Form/InputRadio.php (1)
35-85: Correct and consistent migration fromBaseChoicetoBaseInput.Implementation is symmetrical with
InputCheckbox, differing only inType::RADIO. The trait composition, attribute building, and rendering delegation are all correct.src/Form/InputFile.php (2)
59-69: Correct attribute handling for file inputs.Properly unsets the
valueattribute (disallowed per HTML spec for<input type="file">) and generates the array-suffixed name whenmultipleis set. Clean simplification from the previous unchecked-aware logic.
88-101: Simplified defaults and rendering are consistent with the newBaseInputpattern.
loadDefault()follows the sameparent::loadDefault() + ['type' => [...]]convention used byInputCheckboxandInputRadio. Therun()method now directly delegates tobuildElement().tests/Form/InputCheckboxTest.php (2)
46-60: Attribute ordering change reflects the newBaseInputattribute-building path.The
typenow appears afteridin the expected attributes array, consistent withloadDefault()appending the type to the parent's attributes. Looks correct.
906-915: The test pattern is intentional and consistent with other input types. This file was newly created (not reduced from previous tests), and follows the established pattern of testing only the primary differentiating setter for each input type —checked()for InputCheckbox and InputRadio,alpha()andcolorspace()for InputColor, etc. Since the trait-level methods (autofocus(),required(),tabIndex(),value()) are defined in external packages (ui-awesome/html-attribute, ui-awesome/html-core, etc.), their immutability testing is appropriately delegated to those packages. The current coverage is sufficient and consistent with the codebase design.Likely an incorrect or invalid review comment.
tests/Form/InputRadioTest.php (1)
48-62: Attribute ordering updated consistently withInputCheckboxTest.The
typeattribute now appears afterid, matching theBaseInput-based attribute build order. Correct.tests/Form/InputFileTest.php (2)
41-56: Attribute ordering updated consistently.The expected attributes array reordering (
typeafterid) is consistent with theBaseInput-based attribute build path across all three input types.
925-934: Immutability test now only coverscapture().With the removal of the
uncheckedValue('')assertion, onlycapture('')is tested for immutability. This is acceptable sinceInputFilehas fewer custom trait-provided setters than the checkbox/radio counterparts, andcaptureis the onlyInputFile-specific attribute setter. Other traits (autofocus, required, tabindex, etc.) should ideally be tested for immutability at the trait level.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/Form/InputCheckboxTest.php`:
- Around line 22-29: Update the PHPDoc block in InputCheckboxTest.php to remove
or revise the sentence "Renders label configurations, including content,
attributes, and tag name" so the test coverage summary no longer claims
label-related tests; open the docblock at the top of the InputCheckboxTest class
and either delete that bullet or replace it with an accurate description of
current coverage (e.g., note void element rendering or other existing cases) so
the comment matches the actual tests after CanBeEnclosedByLabel was removed.
In `@tests/Form/InputRadioTest.php`:
- Around line 912-921: The test testReturnNewInstanceWhenSettingAttribute in
InputRadioTest only asserts immutability for checked(true); extend it to also
assert that calling InputRadio::tag()->autofocus(true), ->required(true),
->tabIndex(0) and ->value('x') each return a new instance (not the same object)
to match trait-level immutability coverage—locate the InputRadio class and add
assertions mirroring the existing checked(true) assertNotSame for the methods
autofocus, required, tabIndex and value.
- Around line 22-31: Update the PHPDoc block for the InputRadioTest class to
remove the outdated mention of label tests: locate the docblock in
InputRadioTest (the comment summarizing test coverage) and delete or replace the
line "Renders label configurations, including content, attributes, and tag name"
so the summary only reflects the actual tests (global/custom attributes,
input-specific attributes, checked edge cases, void element rendering, and
provider resolution). Ensure the docblock remains accurate and concise about the
existing test cases.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/Form/InputCheckboxTest.php (2)
364-379: 🧹 Nitpick | 🔵 Trivial
DataProviderExternalusage conflicts with project preference for individual test methods.The project prefers individual, descriptively named test methods over data providers so maintainers can see each case without cross-referencing external provider classes. Consider expanding
testRenderWithCheckedAndValueinto separate methods (e.g.,testRenderWithCheckedAndValueWhenBothMatch,testRenderWithCheckedAndValueWhenMismatch, etc.) and moving the cases inline.Based on learnings from PR
#24: "prefer individual test methods over PHPUnit data providers … Apply to all PHP test files under tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Form/InputCheckboxTest.php` around lines 364 - 379, The test uses DataProviderExternal(CheckedProvider::class, 'checked') in testRenderWithCheckedAndValue which conflicts with the project's preference for individual descriptive test methods; replace the data-provider-driven test by creating multiple small tests named for each case (e.g., testRenderWithCheckedAndValueWhenBothMatch, testRenderWithCheckedAndValueWhenMismatch, etc.) that call InputCheckbox::tag()->id('inputcheckbox')->checked(...)->value(...)->render() and assertSame the expected string inline, moving each provider case into its own method and removing the DataProviderExternal usage and CheckedProvider dependency.
905-914:⚠️ Potential issue | 🟡 MinorImmutability coverage gap for newly added trait methods.
After the pruning, only
checked()is tested for immutability.InputCheckboxnow exposes four new trait methods (autofocus(),required(),tabIndex(),value()) via the refactored design, none of which are asserted to return a new instance here.✅ Suggested additions
public function testReturnNewInstanceWhenSettingAttribute(): void { $inputCheckbox = InputCheckbox::tag(); self::assertNotSame( $inputCheckbox, $inputCheckbox->checked(true), 'Should return a new instance when setting the attribute, ensuring immutability.', ); + + self::assertNotSame( + $inputCheckbox, + $inputCheckbox->autofocus(true), + 'Should return a new instance when setting the attribute, ensuring immutability.', + ); + + self::assertNotSame( + $inputCheckbox, + $inputCheckbox->required(true), + 'Should return a new instance when setting the attribute, ensuring immutability.', + ); + + self::assertNotSame( + $inputCheckbox, + $inputCheckbox->tabIndex(1), + 'Should return a new instance when setting the attribute, ensuring immutability.', + ); + + self::assertNotSame( + $inputCheckbox, + $inputCheckbox->value('value'), + 'Should return a new instance when setting the attribute, ensuring immutability.', + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Form/InputCheckboxTest.php` around lines 905 - 914, Extend the immutability test so it asserts that the other trait setters return new instances too: inside testReturnNewInstanceWhenSettingAttribute add self::assertNotSame checks for $inputCheckbox->autofocus(true), $inputCheckbox->required(true), $inputCheckbox->tabIndex(1) and $inputCheckbox->value('x') (mirror the existing $inputCheckbox->checked(true) assertion) so each of autofocus(), required(), tabIndex(), and value() is verified to return a new instance.tests/Form/InputRadioTest.php (2)
911-920: 🧹 Nitpick | 🔵 TrivialImmutability coverage reduced to a single setter after removing prior tests.
testReturnNewInstanceWhenSettingAttributenow only guardschecked(true). Setters inherited fromBaseInput—form(),name(),value(),required(),disabled(),tabIndex(), etc. — are no longer verified to return new instances. If a future trait accidentally returns$this, no test will catch it.Consider restoring coverage for the core
BaseInputsetters, following the project preference for individual, descriptively named test methods. Based on learnings, this repo prefers individual test methods over providers, and each setter having its own immutability check aligns with that style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Form/InputRadioTest.php` around lines 911 - 920, The current immutability test only checks InputRadio::tag()->checked(true); restore coverage by adding individual test methods that assert each BaseInput setter returns a new instance (use assertNotSame) — e.g., testReturnNewInstanceWhenSettingForm, testReturnNewInstanceWhenSettingName, testReturnNewInstanceWhenSettingValue, testReturnNewInstanceWhenSettingRequired, testReturnNewInstanceWhenSettingDisabled, testReturnNewInstanceWhenSettingTabIndex, etc.; in each test instantiate with InputRadio::tag() and call the corresponding setter (form(), name(), value(), required(), disabled(), tabIndex(), checked() already covered) and assert the original and returned instances are not the same to catch any future accidental return $this regressions.
363-388: 🧹 Nitpick | 🔵 TrivialDocument the
str_replaceassumption to guard against silent drift inCheckedProvider.The
str_replaceworks correctly today becauseCheckedProvider::checkedonly emitstype="checkbox"as an HTML attribute, never inside a value or data-attribute. If the provider ever adds cases wheretype="checkbox"appears in a different context (e.g.,value="type=checkbox") all occurrences will be replaced, silently producing wrong expected strings.A brief inline comment stating the assumption would catch future regressions immediately:
♻️ Suggested documentation addition
- // CheckedProvider returns checkbox-flavored expected HTML; adapt for radio. + // CheckedProvider returns checkbox-flavored expected HTML; adapt for radio. + // Assumption: 'type="checkbox"' appears only as the HTML type attribute in provider output, + // never inside attribute values, so a simple str_replace is safe here. $expected = str_replace(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Form/InputRadioTest.php` around lines 363 - 388, Add a short inline comment immediately above the str_replace call in testRenderWithCheckedAndValue describing the assumption that CheckedProvider::checked only emits the substring type="checkbox" as an HTML attribute (never inside values or data-attributes), so the global str_replace is safe; also mention that if CheckedProvider::checked ever includes type="checkbox" in other contexts the test should be updated to use a more targeted transformation (e.g., attribute-only replacement or a regex) to avoid silent test drift.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/Form/InputCheckboxTest.phptests/Form/InputRadioTest.php
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-02-06T21:37:44.509Z
Learnt from: terabytesoftw
Repo: ui-awesome/html PR: 24
File: tests/Form/InputTextTest.php:33-620
Timestamp: 2026-02-06T21:37:44.509Z
Learning: In the ui-awesome/html repository, prefer individual test methods over PHPUnit data providers in test classes. This helps maintainers see exactly how each test behaves without cross-referencing data providers. Apply to all PHP test files under tests. Use separate, descriptively named test methods to cover different inputs/edge cases; retain data providers only if they clearly improve readability or reduce duplication.
Applied to files:
tests/Form/InputRadioTest.phptests/Form/InputCheckboxTest.php
📚 Learning: 2026-02-11T14:56:18.277Z
Learnt from: terabytesoftw
Repo: ui-awesome/html PR: 37
File: tests/Form/InputTextTest.php:448-457
Timestamp: 2026-02-11T14:56:18.277Z
Learning: In PHP test files under the repository ui-awesome/html (e.g., tests/Form/InputTextTest.php), it is acceptable to use short variable names (e.g., 2-character names like $id). PHPMD's short-variable rule does not need to be strictly followed in test code. During reviews, allow short names in test files and note that this relaxed style applies to tests, while production code should adhere to the standard variable naming conventions.
Applied to files:
tests/Form/InputRadioTest.phptests/Form/InputCheckboxTest.php
📚 Learning: 2026-02-21T01:17:40.877Z
Learnt from: terabytesoftw
Repo: ui-awesome/html PR: 51
File: tests/List/UlTest.php:282-283
Timestamp: 2026-02-21T01:17:40.877Z
Learning: In the ui-awesome/html repository, for PHP tests, ensure that assertion messages in tests/List/UlTest.php for methods named testRenderWith*UsingEnum remain identical to the corresponding non-enum tests (i.e., do not append 'using enum' to the failure message). The reason: the underlying render method treats enum and string inputs the same, so failure output should be uniform. Apply this pattern to all similar test cases under tests/ to maintain consistent failure messages across enum vs non-enum variants.
Applied to files:
tests/Form/InputRadioTest.phptests/Form/InputCheckboxTest.php
⏰ 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). (11)
- GitHub Check: phpunit / PHP 8.2-ubuntu-latest
- GitHub Check: phpunit / PHP 8.3-windows-2022
- GitHub Check: phpunit / PHP 8.5-windows-2022
- GitHub Check: phpunit / PHP 8.5-ubuntu-latest
- GitHub Check: phpunit / PHP 8.4-windows-2022
- GitHub Check: phpunit / PHP 8.2-windows-2022
- GitHub Check: phpunit / PHP 8.1-windows-2022
- GitHub Check: phpunit / PHP 8.1-ubuntu-latest
- GitHub Check: linter / Super Linter
- GitHub Check: mutation / PHP 8.5-ubuntu-latest
- GitHub Check: linter / Super Linter
🔇 Additional comments (2)
tests/Form/InputCheckboxTest.php (1)
45-59: Attribute-order assertion looks correct for the newBaseInputinheritance path.
id(from parent defaults) precedingtype(set byInputCheckbox::loadDefault()) accurately mirrors the merged attribute map produced by the refactored class. TheassertSameon the ordered array will correctly catch any future re-ordering regression.tests/Form/InputRadioTest.php (1)
47-61: No changes needed. The attribute insertion order (id→type→class) is consistent across all Input element tests (InputText, InputCheckbox, InputEmail, etc.) and reflects the stable behavior ofparent::getAttributes()followed bybuildAttributes(), which only processes thevalueattribute without reordering keys. This pattern is intentional and correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/Form/InputCheckboxTest.php`:
- Around line 364-379: The test uses
DataProviderExternal(CheckedProvider::class, 'checked') in
testRenderWithCheckedAndValue which conflicts with the project's preference for
individual descriptive test methods; replace the data-provider-driven test by
creating multiple small tests named for each case (e.g.,
testRenderWithCheckedAndValueWhenBothMatch,
testRenderWithCheckedAndValueWhenMismatch, etc.) that call
InputCheckbox::tag()->id('inputcheckbox')->checked(...)->value(...)->render()
and assertSame the expected string inline, moving each provider case into its
own method and removing the DataProviderExternal usage and CheckedProvider
dependency.
- Around line 905-914: Extend the immutability test so it asserts that the other
trait setters return new instances too: inside
testReturnNewInstanceWhenSettingAttribute add self::assertNotSame checks for
$inputCheckbox->autofocus(true), $inputCheckbox->required(true),
$inputCheckbox->tabIndex(1) and $inputCheckbox->value('x') (mirror the existing
$inputCheckbox->checked(true) assertion) so each of autofocus(), required(),
tabIndex(), and value() is verified to return a new instance.
In `@tests/Form/InputRadioTest.php`:
- Around line 911-920: The current immutability test only checks
InputRadio::tag()->checked(true); restore coverage by adding individual test
methods that assert each BaseInput setter returns a new instance (use
assertNotSame) — e.g., testReturnNewInstanceWhenSettingForm,
testReturnNewInstanceWhenSettingName, testReturnNewInstanceWhenSettingValue,
testReturnNewInstanceWhenSettingRequired,
testReturnNewInstanceWhenSettingDisabled,
testReturnNewInstanceWhenSettingTabIndex, etc.; in each test instantiate with
InputRadio::tag() and call the corresponding setter (form(), name(), value(),
required(), disabled(), tabIndex(), checked() already covered) and assert the
original and returned instances are not the same to catch any future accidental
return $this regressions.
- Around line 363-388: Add a short inline comment immediately above the
str_replace call in testRenderWithCheckedAndValue describing the assumption that
CheckedProvider::checked only emits the substring type="checkbox" as an HTML
attribute (never inside values or data-attributes), so the global str_replace is
safe; also mention that if CheckedProvider::checked ever includes
type="checkbox" in other contexts the test should be updated to use a more
targeted transformation (e.g., attribute-only replacement or a regex) to avoid
silent test drift.
1 participant
Pull Request