8000 ChoiceFormField::isDisabled for checkbox · Issue #8348 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

ChoiceFormField::isDisabled for checkbox #8348

New issue

Have a question about this project? Sign up for a free GitHub account to open an i 8000 ssue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom
Closed
tamirvs opened this issue Jun 24, 2013 · 9 comments
Closed

ChoiceFormField::isDisabled for checkbox #8348

tamirvs opened this issue Jun 24, 2013 · 9 comments
Labels
Bug DomCrawler Keep open

Comments

@tamirvs
Copy link
Contributor
tamirvs commented Jun 24, 2013

Checking if a checkbox is disabled using isDisabled() returns false even when the checkbox really is disabled.

I looked through the code and I think the problem is this line because when I dumped the ChoiceFormField checkbox object this is what I got:

['options':private] =>
    array {
        array {
            'value' => 1,
            'disabled' => true
        }
    }
['value':protected] =>
    null

So obviously this $option['value'] == $this->value is false..
If when building the form I add an option 'value' => null to the checkbox form it works just fine.

Shouldn't the checkbox isDisabled function be different than the one for other choice inputs?
@jfsimon
Copy link
Contributor
jfsimon commented Jul 15, 2013

@tamirvs The ChoiceFormField::isDisabled() method Check if the current selected option is disabled.

@tamirvs
Copy link
Contributor Author
tamirvs commented Jul 16, 2013

so it's not meant to be used on checkbox?

@craigmarvelley
Copy link

Looking at the original commit this method was designed for use with radio buttons. As @jfsimon says its name is misleading because it only pays attention to the currently selected option - if no options are selected, it is supposed to return false. I'd expected it to return true if the actual field is disabled, but it's looking at the options within. Maybe it's named inappropriately, and should be titled isDisabledOption or selectedOptionIsDisabled or something similar?

In any case, I'm not sure the way this method works extends to all the different ways the class can be configured (especially as it's possible for multi-checkboxes / radios to be configured via the class).

We could provide an optional parameter to the method which would dictate how to assert the disabled status - possible values being

ChoiceFormField::OPTION_SELECTED
ChoiceFormField::OPTION_ANY
ChoiceFormField::OPTION_ALL

the former would be the default, maintaining the current behaviour for the case of single selects. In the case of multiple selects, all of the selected options would have to be disabled for true to be returned.

The middle case would return true if any of the options were disabled, which for single selects would provide the behaviour expected by @tamirvs. The same for the latter case, which would be more useful for fields where multiple options can be selected.

Happy to do a patch if this sounds sensible to someone else.

@tamirvs
Copy link
Contributor Author
tamirvs commented Dec 10, 2013

@craigmarvelley +1

@jakzal
Copy link
Contributor
jakzal commented Feb 12, 2016

The ChoiceFormField::isDisabled() method checks if a currently selected option is disabled.

In case of a radio button this might make sense, as all the radio buttons share the same name.

In case of the checkbox I think the value shouldn't matter. There's always a single checkbox in each ChoiceFormField. Therefore I think #2720 broke the behaviour for checkboxes.

In case of a select with options it's a bit more complex, as both the select node can be disabled, as well as each of the options. Current logic will only check the currently selected option, and ignore the select node.

I suggest we don't verify option's value for checkboxes.

@xabbuh
Copy link
Member
xabbuh commented Feb 17, 2016

The handling of select nodes should be fixed by #17542 I guess.

@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@carsonbot
Copy link

Friendly ping? Should this still be open? I will close if I don't hear anything.

@Nyholm
Copy link
Member
Nyholm commented Jan 17, 2021

Yes, it should still be open.

@carsonbot carsonbot removed the Stalled label Jan 17, 2021
@MatTheCat MatTheCat mentioned this issue Jan 7, 2025
Merged
nicolas-grekas added a commit that referenced this issue Jan 22, 2025
@nicolas-grekas
bug #59399 [DomCrawler] Make ChoiceFormField::isDisabled return `tr…
cb5fdaf 
…ue` for unchecked disabled checkboxes (MatTheCat)

This PR was merged into the 6.4 branch.

Discussion
----------

[DomCrawler] Make `ChoiceFormField::isDisabled` return `true` for unchecked disabled checkboxes

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #8348
| License       | MIT

`ChoiceFormField::isDisabled`’s PHPDoc reads

> Check if the current selected option is disabled.

But a checkbox really embeds two options: either you check it, or not; which means it always has a selected option.
Then, if a checkbox is disabled, these two options are too.
So, `ChoiceFormField::isDisabled` should return `true` for disabled checkboxes, checked or not.

This also matches what you intuitively would expect from this method.

Commits
-------

9807ce6 [DomCrawler] Make `ChoiceFormField::isDisabled` return `true` for unchecked disabled checkboxes
@xabbuh xabbuh added Bug and removed DomCrawler labels Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DomCrawler Keep open
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@javiereguiluz @jfsimon @jakzal @nicolas-grekas @craigmarvelley @Nyholm @tamirvs @xabbuh @carsonbot
0