Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
As noted http://drupal.org/node/335035#comment-1446380, it seems odd that
$form['some_field'] = array(
'#disabled' => TRUE,
);
merely changes the theme settings (as seen in documentation: http://api.drupal.org/api/file/developer/topics/forms_api_reference.html...).
It would seem useful to have the form API ignore field values when #disabled is TRUE. Maybe I'm missing some reasons for not doing this?
Comment | File | Size | Author |
---|---|---|---|
#51 | 426056-fapi-disabled.51.patch | 13.61 KB | tstoeckler |
#46 | 426056-fapi-disabled.46.patch | 13.61 KB | effulgentsia |
#36 | 426056-fapi-disabled.36.patch | 12.14 KB | effulgentsia |
#35 | 426056-fapi-disabled.35.patch | 11.52 KB | effulgentsia |
#30 | 426056-fapi-disabled.30.patch | 4.92 KB | effulgentsia |
Comments
Comment #1
Dave ReidWhat should happen in the case that I add #disabled = TRUE to my form element, but then have a JavaScript that will un-disable the field on a certain condition. How will FAPI know when it can or cannot accept the input value?
Comment #2
boombatower CreditAttribution: boombatower commentedWould seem if JavaScript is enabled then you should disable/enable the field on the client side if it is dynamic, otherwise if #disabled is set it is final.
I feel this is the same as the Form API testing for invalid select options.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedSubscribing. This may end up being a blocker for #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use....
I agree with @boombatower that JavaScript isn't a problem. If JavaScript wants to "pseudo-disable" a field that isn't actually disabled, it can do so on $(document).ready() and then enable/disable it at will from that point forward. But if a field is defined with #disabled in Form API, then it seems like the server should enforce that that's actually the case.
Comment #4
sunCorrect, and this is a bug.
Attached patch implements a test that proves this. On that note, #required, but #disabled textareas seem to be handled properly already (i.e. the last 2 assertions pass).
Now it's up to FAPI masters to make those tests pass. :)
Comment #6
BerdirHm..
I'm not a FAPI master, but I want to fix that edit bug :)
I am not sure about the validation stuff, do we really want to display a form error if the value has been changed or is it enough if we silently enforce the #default_value?
Because doing the latter is a simple, 1-line change, adding error messages does make it quite a bit more complicated... I don't yet fully understand how that value gets set and where it does come from.
Attached patch just enforces the default value, let's see if will break something else... Atleast the form validiation assertions will still fail, obviously..
Comment #8
BerdirOk, here is a new try. This time, with validation errors. I updated in _submitForm() to use the new form_clear_errors() function to clear the form error function cache.
All tests should now pass...
Comment #9
sunLet's move the primary condition to the start... :)
"Issue a validation error, when an element is disabled and its #value is different than its #default_value."
At least there should be a trailing period (full-stop) in the error message. However, I'm also not sure whether that message is not a bit too technical...?
Beer-o-mania starts in 18 days! Don't drink and patch.
Comment #10
BerdirUser Joe Common will most probably never see this message. I can see two reasons why that message would be displayed
a) Someone tries to do something "bad", like removing the required attribute with FireBug or a script and tries to send a value with it. We don't need a valiation message at all, here..
b) Someone develops a module and uses #disabled but does then add some javascript to un-disable it. Here would a technical validation error be really useful.
I also talked with chx about this and he was against a validation message (and not really for this patch at all, either ;))
(sorry for quoting without asking, you are sleeping :))
We also need to update the documentation and write what #disabled means. Because http://api.drupal.org/api/file/developer/topics/forms_api_reference.html... says nothing about protection, only that the element is greyed out. However, if you just want that, you can also use $form['element']['#attributes']['disabled'].
The updated patch fixes the string/documentation issues and also removes the special handling from checkbox. That we are doing it for those imho really means that we should do it for all form elements. The only place in core where we used that "un-disabling" were the teaser fields AFAIK.
Comment #12
BerdirThere were some tests that called the checkbox_value() function directly. removed them and added one more test for checkbox fields...
Comment #13
boombatower CreditAttribution: boombatower commented@chx in #10: In the same way that you get a validation error if you tamper with a select so should you if you tamper with a disabled field. Seems simple and clean, and if a developer/user accidentally does it at least they get something instead of just wondering why the value keeps saving as blank.
Comment #14
boombatower CreditAttribution: boombatower commentedStill needs additional reviews, most of this is minor.
Would seem more standard to use $extras => $extra.
Why assign #default_value here if always overridden by $extras.
Pre our conversion away from t(), but I'm not apposed to leaving it off assertion messages since we have already agreed upon it.
Since we are using classes we don't need "_", private property is enough.
18 days to code freeze. Better review yourself.
Comment #15
BerdirRe-rolled with the things mentioned in #14 fixed.
Comment #16
boombatower CreditAttribution: boombatower commentedTested manually by editing a few forms and read through the code...looks good. Only comment I have is included below. I tweaked in patch.
Seems like bad form to have same variable name...although it appears to work, not my first guess.
I'm on crack. Are you, too?
Comment #17
boombatower CreditAttribution: boombatower commentedMeant to mark it.
Comment #18
Dries CreditAttribution: Dries commentedSo, is chx still against this or? It is not clear whether we reached consensus.
Comment #19
chx CreditAttribution: chx commentedI _think_ disabled is a display gimmick and we actually should remove the checkboxes special handling of it. One can set #value on an element to prevent tampering, or use #access FALSE... #disabled is neither of them. Actually, there are other ways of making a form element usually unchangeable in a browser, readonly or you can hide it with visibility: hidden or display:none. These are visibility gimmicks and we should not cover them. I think.
Comment #20
boombatower CreditAttribution: boombatower commentedAfter discussing it with chx in person I agree that #access and other methods make sense to use for form elements that you don't want changed...and that #disabled is purely for cosmetics.
Comment #21
chx CreditAttribution: chx commentedUsually I post more to clarify my position further. Not in this case. There is no clear cut "this is correct" solution in this case. The option checker throws a validation error which is cryptic and largely useless to the user however there is something at least. We can do the validation message here as proposed if people think that's better. We can throw away simply what the user entered. We can disregard #disabled. Hpwever, as Berdir points out, attributes['#disabled'] can be used for the meaningless UI role so the last option is probably not necessary. I let the community decide what do they want. I do not have strong feelings , either of the three is fine.
Comment #22
BerdirI do not care if we have a validation message or not, so I left it in.
In the patch, I changed a few things to have the #disabled handling after #process functions, so that it can be used for #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use. too. If this does not work, then this patch isn't that important anyway... ;) Not sure if the tests will pass, I have some weird issues locally.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedThis issue has resurfaced due to a bug initially reported in field_ui.module: #687572: When disabled textfields are submitted, their values are forced to empty string instead of the default value. We should probably mark either this one or that one a dupe. That one has an up to date patch. Would folks like to continue the discussion there, or should I close the other one, and move relevant comments and patch to here?
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedNever mind about #24. I updated the other issue with a patch that's specific to that issue. Here's an updated patch for this issue (without tests and without validation message). I agree with earlier comments that #disabled should be a FAPI concept (meaning this element really is disabled and don't let client-side code get around that) and that
#attributes['disabled']='disabled'
should be used by developers who want the element to start off greyed out in the browser, but that shouldn't be considered disabled by FAPI.1. Is there consensus that this is indeed a bug that needs fixing?
2. What's the consensus on whether to re-roll the patch to include a validation error message instead of simply disregarding the input?
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedRelated issue: #500868: Forms API is missing the readonly form field option. That one introduces an interesting problem: if HTML supports two different attributes that implement different UI for the same functionality desired server-side, what should we do?
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedHere's a thought on how to support the HTML distinction of 'readonly' vs. 'disabled' while still maintaining a single property #disabled that is used during validation (so that hook_form_alter()'s have one property rather than two to control whether input is accepted).
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedChanged proposed new property name to #allow_focus instead of #disabled_allow_focus.
Comment #29
Eric_A CreditAttribution: Eric_A commentedHow about integer 0? My point being that string '0' is very numeric to is_numeric()...
EDIT: my bad, this was in the original code already...
Comment #30
effulgentsia CreditAttribution: effulgentsia commentedIt's a good point though, so this patch fixes it.
I hope this issue gets resolved before D7 release, but if not, I just realized that it is possible to work around it by having forms set #value on elements that they don't want user input overriding, and potentially this should be the preferred way of identifying an element for which input isn't wanted, leaving #disabled as only a UI default rather than having any server-side meaning. But if that's our decision, we should document it, including updating http://api.drupal.org/api/drupal/developer--topics--forms_api_reference.... to make it clear that all elements support a #value property.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedI'm fine with either the latest patch or "#disabled as only a UI default rather than having any server-side meaning". since we have a patch for one approach, lets go with that.
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedAnd just to clarify/summarize, as alluded to in #10 and #19, HEAD currently has an inconsistency in that for checkboxes and checkboxes only, #disabled means server-enforced disabled, while for all other controls, #disabled is a UI default only. #30 makes all controls treat #disabled the same way that HEAD currently does for checkboxes. Other options have been discussed in earlier comments, and in #21, chx indicated neutrality on which option is chosen as long as it is consistent. There seems to be more desire from the people who participated in this issue to make #disabled mean server-enforced disabled (making everything more like checkboxes rather than making checkboxes more like everything else). Earlier patches implemented validation errors (which would be new functionality in that HEAD does not set validation errors if input is submitted for a disabled checkbox, the input is simply silently ignored). I'm not clear whether consensus was reached on that, so #30 doesn't implement validation errors, it simply makes all controls treat #disabled the same way that checkboxes currently do. I'm certainly open to merging the code from earlier patches that implement validation errors if there's consensus that it's desired and that doing so is in-scope for D7.
Comment #33
webchickThanks for the great summary! I agree that a logical assumption for a developer is that a FAPI property #disabled would have some 'magic' behind it to auto-validate its value, especially since checkboxes do this.
However, as with anytime we go monkeying around in the innards of FAPI, we need to have tests for this. It looks like there were some earlier that sun posted; can we just integrate them into the new patch?
Comment #34
sunAwesome job, effulgentsia!
Reading both patches/approaches, I really prefer #30. Recalling when I got bit by this issue myself, I was quite confused that #disabled doesn't really mean "no input allowed here". I expected that the form element input value would be ignored, no matter what. So effulgentsia's patch brings Form API to (at least my) expectations.
And if you want a semi-#disabled element that can still be enabled, you can use #attributes. But then again, it's also highly debatable and highly depends on the actual form/workflow, whether JavaScript shouldn't set the initial 'disabled' attribute instead. In that manner, a disabled form element is indeed pure cosmetics without any required server-side logic, so the developer has to decide on a case-by-case basis.
My original tests can be found in #4. However, I'm not sure whether they are still valid, might require some tweaking.
Comment #35
effulgentsia CreditAttribution: effulgentsia commented@sun: are you okay with this as the test instead of what you had? This test causes several failures in HEAD (as expected) and succeeds with the patch. And just for additional confirmation that test-driven development is good, it identified 2 more hunks needed in form.inc to deal with checkboxes and radios.
Comment #36
effulgentsia CreditAttribution: effulgentsia commentedSo true, and we shouldn't be giving tacit approval for developers to write inaccessible forms, so this patch expands the comment to mention this.
Comment #37
Eric_A CreditAttribution: Eric_A commentedHmm, It looks like we encounter problematic behaviour when disabling a checkbox with an undefined #default_value? Seems the patch replaces old problematic behaviour with new problematic behaviour.
The submit handler expects integer 0 or the #return_value string, but:
Before patch:
Notice: Undefined index: #default_value in form_type_checkbox_value() ... (See #642820: PHP notice when submitting form with disabled checkbox)
($form_state child element populated with the NULL value, I guess. EDIT: nope, it's ''. First NULL is passed to form_type_checkbox_value(), then FALSE. Nothing is returned and since there is no default value we end up with ''.)
After patch:
$form_state child element populated with the empty string. Auch.
I'm not 100% sure at this moment so I'm leaving status as is for now.
Also, I have no idea if single checkboxes without #default_value exist in core. If not, this behaviour will not surface with core, but if so, then any contrib module that disables one will run into a WTF with core.
Comment #38
Eric_A CreditAttribution: Eric_A commentedAuch, seems I saw things simply wrong. NULL values are set as input so we get either 0 or #return_value...
EDIT: Well, now I'm thinking I was right after all. Seems the above mentioned logic is skipped when an element is disabled and we're simply passing FALSE as input to the value callback. Then nothing is returned and $form_state is populated with either #default_value (when set) or ''.
I cannot do any testing now, just reading code...
Comment #39
Eric_A CreditAttribution: Eric_A commentedBetter description of #37 and #38:
This patch reveals bad behaviour of an unchecked checkbox when there is no default value and #disabled == TRUE.
(#access == FALSE probably triggers the exact same logic as #disabled == TRUE, so I'm thinking the problem exists there as well).
This patch makes visible that form_type_checkbox_value() does not return the same value for these 2 scenario's with an unchecked checkbox and no default value:
1) unchecked, enabled and accessible; $input === NULL; should return 0 and it does.
2) unchecked, disabled; $input === FALSE; should return 0 but it returns nothing; In the end #value will be set to ''. This is very problematic when you happen to use '' as your checkbox value.
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedYeah, good catch, separate but related issue. Therefore, let's resolve #642820: PHP notice when submitting form with disabled checkbox first, then come back to this one.
Comment #41
effulgentsia CreditAttribution: effulgentsia commentedThere's another dependency before this issue can proceed: #712990: Test framework does not correctly emulate browser submission of disabled elements
Comment #42
thekevinday CreditAttribution: thekevinday commentedOkay, so here is my argument on this matter.
I believe that (for numerous reasons, such as accessibility support) the use of disabled and readonly should be treated exactly as is in accordance to the web standards.
A person should not have to expect custom or non-standard stuff to happen when they use standard DOM.
The end result of adding custom functionality to standards is proven by Internet Explorer.
That said, there is an obvious need (if not requirement) for programmatic interpretation of data.
Drupal is a CMS, obviously.
Instead of breaking web standards (per say), why not add another option that can provide the needs as defined in this issue while still supporting web standards exactly as they are?
Lets say we have a checkbox called "Ignore Field Input If.."
Which is followed by a multi-select drop-down with:
- ReadOnly
- Disabled
When the check box is selected, Disabled is selected in the list box, and the Disabled checkbox is checked, then:
- When a user saves a form, all disabled fields will not trigger any validation and the disabled data will be dropped in the event that something on the user end injects something into that field.
The same would be true for readonly.
If the user wants to have the data in the Disabled or ReadOnly fields be sent to the server, then they MUST be validated for security purposes (ie: js injection attacks).
Therefore when the "Ignore Field Input If.." check box is unselected, validation will still happen and the value of the field will be passed to the server.
The use case for having a disabled field in this way is the use of Conditional Fields.
Lets say I have a case where I have a Conditional Field that is dependent on having a Controlling Field be set to a certain value.
If that field is set to the appropriate value, then the Conditional Field can be edited by the user.
If the Controlling Field is set to something else, I might force a default and disable the users ability to change that value.
I still need to interpret the value and pass it to the server.
I may or may not want the user to be able to copy and paste data from within the disabled field if they are allowed to see that field.
This example can become even more complex if the situation becomes dependent on multiple fields and each of those multiple fields applies their own twists.
Forms is a drupal core project and not an extra module, so it must be taken into consideration that modules will override things and add extra complexity (as in the provided use case).
One must also consider that drupal is a web-based application and therefore one would expect its users to expect drupal to follow and provide something that follows web standards (without gotchas).
What I am trying to get at is:
Don't force added interpretation that is outside of a standard, but feel free to provide the end-user to select an option to do so.
Please add an extra option similar to (if not the same as) the "Ignore field.." concept I presented here instead of forcing all Disabled data to be ignored.
And just to be thorough, here is what my favorite source claims how Disabled and ReadOnly fields should function:
(http://w3schools.com/tags/att_textarea_disabled.asp) (http://w3schools.com/tags/att_textarea_readonly.asp)
disabled
readonly
Comment #43
effulgentsia CreditAttribution: effulgentsia commented@thekevinday: sorry, I'm not quite following. Can you summarize a little more succinctly what you're requesting?
It's already the case that every form element has a #attributes, and that's where you can put stuff to affect HTML attributes. So, if you want to apply the "disabled" or "readonly" HTML attributes to an element, without that affecting server-side logic, you can do so. If the conditional field module wants to start an element off as disabled to make the UI better, but not really have it be disabled as far as server-side processing is concerned, it can do so by setting $element['#attributes']['disabled']. Though if it's interested in making the UI work well for users with JavaScript disabled, it may prefer to not do that, and instead, to use JavaScript to disable the necessary elements on document ready.
The #disabled property is not an HTML attribute, it is a Form API property. It means to disable the element as far as the Form API is concerned, which means: server-side code does not want user input for this element, but since PHP can't prevent a rogue user agent from submitting the input, the Form API needs to take steps to ensure that the rogue input doesn't get used where it shouldn't be. This isn't violating HTML conventions, it is choosing to use the word "disabled" to mean something appropriate for what the Form API is. This is similar to the #type property, which has meaning in Drupal rendering that is different from the meaning of the HTML 'type' attribute. The English language is limited, and sometimes the precise meaning of a word depends on its context and Form API is a different context than HTML.
By the way, this issue is still postponed due to #335035: drupalPost() incorrectly submits input for disabled elements being needed before the correct tests can be added to this patch.
Comment #44
thekevinday CreditAttribution: thekevinday commentedSorry, I sort of got redirected here and was thinking of this in the same context as the other post I was on. (for reference, see: http://drupal.org/node/500868)
EDIT:
After spending a few days width watchdog(), hook_form_alter(), and callback_after_build(), I managed to make since of how disabled and readonly are actually being handled.
If I edit the
$form[$key][0]['value']['#attributes']['disabled']
which ultimately boils down to the form API's$form['#attributes']['disabled']
, will the '#disabled' discussed in this thread be affected by the 'disabled' inside of '#attributes' (and vice-versa)?Or that was what I was asking, so your first statement answered this.
Thanks for clearing this up!
Comment #45
effulgentsia CreditAttribution: effulgentsia commented@thekevinday: cool. I'm glad we're on the same page!
#335035: drupalPost() incorrectly submits input for disabled elements landed, so setting back to "needs work". I'm gonna try to roll an updated patch with proper tests soon.
Comment #46
effulgentsia CreditAttribution: effulgentsia commentedRe-rolled and with test. Moshe or sun, please RTBC or suggest changes. Thanks!
Comment #47
effulgentsia CreditAttribution: effulgentsia commentedBetter title, see #32. This patch does not consider it a validation error if input is submitted for a disabled element. It just makes FAPI not process that input into #value and $form_state['values']. This is already what HEAD does for checkboxes. This patch applies that logic for all elements.
Comment #48
moshe weitzman CreditAttribution: moshe weitzman commentedThe inline docs here approach the level of poetry. Very nicely done. Thanks.
Comment #49
effulgentsia CreditAttribution: effulgentsia commentedBugs related to inconsistencies that touch on FAPI security seems to qualify as "critical" to me.
Comment #50
sunCritical bump.
Comment #51
tstoecklerExtra whitespace.
No other changes. Leaving at RTBC.
Powered by Dreditor.
Comment #52
fleetcommand CreditAttribution: fleetcommand commented(subscribe)
Comment #53
Eric_A CreditAttribution: Eric_A commentedRemoved "Needs tests" tag...
Comment #54
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks all. :)
Comment #55
EugenMayer CreditAttribution: EugenMayer commentedAny reason why this is D7 only?
Comment #56
effulgentsia CreditAttribution: effulgentsia commented@#55: It would be a backwards compatibility break for D6. Breaking BC of an already released major Drupal version should only be done if necessary to fix a real security hole or similarly important bug. While ensuring consistency of the server-side meaning of #disabled across element types is good for DX and makes it less likely that module authors will accidentally expose security holes in their forms, it's not clear to me that D6 functionality is actually insecure (there may be modules that are, if they're incorrectly assuming that FAPI is enforcing #disabled during input processing, but that can be fixed by the module to not assume that). If someone finds a real security hole in Drupal 6, and porting this fix to D6 is the best way to solve it, then this issue's version should be changed to "6.x-dev" and the status should be changed to "patch (to be ported)".
Comment #57
EugenMayer CreditAttribution: EugenMayer commentedFine
Comment #58
effulgentsia CreditAttribution: effulgentsia commentedBy the way, in case you didn't know, in forms where you want to control what gets into $form_state['values'] regardless of what gets submitted in $_POST, you can set #value. So anywhere in D6 where you want #disabled to have the meaning it now has in D7, you can do:
Not as elegant or intuitive as #disabled working as one would expect, but a way for D6 modules to ensure security where it's needed.
Comment #60
arpeggio CreditAttribution: arpeggio commentedRe-openning... Same issue mentioned here 803212: Protection against forgery of input selection value doesn't work with checkboxes post #12 are the steps to reproduce the issue in D7.
Comment #61
effulgentsia CreditAttribution: effulgentsia commentedThis is being addressed in #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security.
Comment #62
ledut CreditAttribution: ledut commentedsubscribing...
Comment #63
Blooniverse CreditAttribution: Blooniverse commentedVerrry sorrry for reopening! Reason: @see #1837306: Textfield input attrib #disabled doesn't work! !
Comment #64
Blooniverse CreditAttribution: Blooniverse commented... at first glance, I would say there is something wrong with
if (!empty($element['#allow_focus'])) {
in the patch of comment#51.Actually, this whole paragraph below looks odd to me! Why?: The two HTML element attributes are in reality not linked together (remark: this is a practical/experiential point of view; I didn't take the time to check the browser implementation/specs)! In current Drupal code (v7.17) they seem to be linked, though! Really,
'disabled'
is'disabled'
&'readonly'
is'readonly'
. You might find the following summary about the meaning of both elements concisely useful: http://htmlcodetutorial.com/....Original patch paragraph (diff merged)
Well, I have to admit to not having had enough time to check the patch properly. It's just that in my imagination the code should look really simple, almost like:
New paragraph
((Btw, if I disable a textfield, it gets submitted nonetheless!))
Comment #65
tim.plunkettPlease open a new issue, or continue work in the issue you linked.