Skip to content
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

Enhance select element #88

Merged
merged 13 commits into from
Jan 10, 2023
Merged

Enhance select element #88

merged 13 commits into from
Jan 10, 2023

Conversation

sukhwinder33445
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 commented Nov 8, 2022

Blocked by

fixes #84

TODO:

  • Fix that test testSelectingDisabledOptionIsNotPossible() fails because of cached isValid().

@cla-bot cla-bot bot added the cla/signed label Nov 8, 2022
@sukhwinder33445 sukhwinder33445 force-pushed the enhance-select-element branch 2 times, most recently from 8dda69f to e71d5f3 Compare November 8, 2022 11:55
@nilmerg nilmerg added this to the v0.7.0 milestone Nov 8, 2022
@nilmerg nilmerg added the enhancement New feature or request label Nov 8, 2022
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are adjusting the entire class, please define the proper arguments and return types for all methods the class provides itself.

src/FormElement/SelectElement.php Outdated Show resolved Hide resolved
src/FormElement/SelectElement.php Outdated Show resolved Hide resolved
src/FormElement/SelectElement.php Outdated Show resolved Hide resolved
src/FormElement/SelectElement.php Outdated Show resolved Hide resolved
src/FormElement/SelectElement.php Outdated Show resolved Hide resolved
tests/FormElement/SelectElementTest.php Outdated Show resolved Hide resolved
src/FormElement/SelectElement.php Outdated Show resolved Hide resolved
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please document the trait.
  2. Please test the trait.
  3. Don't drop ZF1 compatibility yet. I haven't checked if any of this applies to IPL forms, but I wouldn't bother to check either. The separate PR was fine.
  4. The whole logic for disabling options is questionable as it is not possible to control this via construct time attributes and option groups cannot be disabled or the entire element (which effectively means all options (and groups) are automatically disabled). And the funniest thing is, it's not used at all, see here, here, here and here. So we have a semi-implemented feature that is not used at all and just presents complexity for further changes. I would remove the ability to disable options.
  5. And while we're removing stuff anyway, please drop deselect(). Also not used.

Update:

4. I would remove the ability to disable options.

This applies to the methods in the form, since $form->getOption()->setAttribute('disabled', true) can always be called. Note that validation must still take disabled attributes into account. Your current changes would no longer do that for the above call. Please add a test for this as well.

@sukhwinder33445 sukhwinder33445 force-pushed the enhance-select-element branch 3 times, most recently from fafe57f to ee0b1bd Compare November 11, 2022 12:03
nilmerg added a commit to Icinga/ipl-validator that referenced this pull request Dec 6, 2022
nilmerg added a commit to Icinga/ipl-validator that referenced this pull request Dec 6, 2022
src/FormElement/SelectElement.php Outdated Show resolved Hide resolved
@cla-bot
Copy link

cla-bot bot commented Dec 21, 2022

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @sukhwinder33445

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

  • If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

@cla-bot cla-bot bot removed the cla/signed label Dec 21, 2022
@sukhwinder33445
Copy link
Contributor Author

@cla-bot check

@cla-bot cla-bot bot added the cla/signed label Dec 21, 2022
@nilmerg nilmerg removed the request for review from lippserd January 10, 2023 08:38
@nilmerg nilmerg merged commit b563138 into master Jan 10, 2023
@nilmerg nilmerg deleted the enhance-select-element branch January 10, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disabled select element has null as value on submit
3 participants