In field_default_form(), it only populates the form if field_access() returns true, this means you get a completely different form structure depending on whether access is true or false, and any code which might depend on that form structure can get horribly broken.
Instead we should have the same form structure, but set #access => FALSE.
Attaching a test (which should fail with one fail and two exceptions) first, then test and patch forthcoming.
Comment | File | Size | Author |
---|---|---|---|
#68 | drupal-822418-68.patch | 11.49 KB | tim.plunkett |
#61 | drupal-822418-61-combined.patch | 11.29 KB | tim.plunkett |
#61 | drupal-822418-61-tests.patch | 1.14 KB | tim.plunkett |
#57 | 822418-56.patch | 11.34 KB | no_commit_credit |
#57 | interdiff.txt | 997 bytes | no_commit_credit |
Comments
Comment #1
catchAnd the full patch.
Comment #2
yched CreditAttribution: yched commentedHm, I think we went back and forth on this in, IIRC, #629252: field_attach_form() should make available all field translations on submit.
Trying to get my brain in 'remember' mode.
Comment #3
yched CreditAttribution: yched commentedActually, this very change (or something very similar) went in with #636834: Field revision data messed up when user has no 'edit' access on the field.
And #629252-22: field_attach_form() should make available all field translations on submit was the reason this got changed back - quoting / adapting from there:
Imagine an integer field, with a 'min value' setting of '0'.
Create a node with value '1' for the field.
Then edit the field and set the min value to '2'.
If a user doesn't have 'edit' access for the field, and we include the widget in the form with #access = FALSE, any attempt at editing the node will fail validation, because there are some submitted values that are invalid.
And the user cannot do anything about it, because the invalid value is not accessible to him.
Comment #4
catchThat sounds like correct behaviour to me to be honest, a bit annoying if you're that user, but it's the fault of the administrator, not Drupal, that you get into that situation. Certainly it's more of an edge case than having code which works without field access then fails once you add it.
Comment #5
yched CreditAttribution: yched commented"it's the fault of the administrator, not Drupal, that you get into that situation"
I don't really see how. The admin used the UI to do perfectly valid changes.
from #629252-23: field_attach_form() should make available all field translations on submit :
"Hm. Unless we explicitly make field_default_form_errors() *not* report errors on elements where #access == FALSE".
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedHmmm. I think I could go either way on this one. Seems like a reasonable compromise to add "field_default_form_errors() *not* report errors on elements where #access == FALSE"
Comment #7
catchThat seems like a decent compromise to me too. My main concern is that hook_field_attach_submit() gets the same $form and $form_state each time it's called - the way this was found was a hook implementation added four months ago, which worked fine for four months, then suddenly stopped working when hook_field_access() was implemented and the usual array structure disappeared from under it. I'll look at field_default_form_errors() a bit later today and see if I can add that to the patch.
Comment #8
chx CreditAttribution: chx commentedbump. still buggy.
Comment #9
bryancasler CreditAttribution: bryancasler commentedsubscribe
Comment #10
gagoo CreditAttribution: gagoo commentedsubscribe
Comment #11
clashar CreditAttribution: clashar commentedsubscribe, came from #1062072: Notice : Undefined property: stdClass::$field_name_of_the_field in locale_field_node_form_submit()
Comment #12
ray17n CreditAttribution: ray17n commentedsubscribe
Comment #13
restyler CreditAttribution: restyler commentedsubscribing from #1062072: Notice : Undefined property: stdClass::$field_name_of_the_field in locale_field_node_form_submit()
Comment #14
tobey_p CreditAttribution: tobey_p commentedsubscribe
Comment #15
sunComment #16
sun#1: 822418_field_access_form.patch queued for re-testing.
Comment #18
snupy CreditAttribution: snupy commentedsubscribe
Comment #19
marcingy CreditAttribution: marcingy commentedReroll of patch to head
Comment #20
marcingy CreditAttribution: marcingy commentedComment #22
marcingy CreditAttribution: marcingy commentedBad reroll missed a property lets try again.
Comment #24
marcingy CreditAttribution: marcingy commented#22: form-field-acess-822418-22.patch queued for re-testing.
Comment #25
xjmTagging issues not yet using summary template.
Comment #26
ArlaTo quickly fix this problem on my D7 site, can I apply form-field-acess-822418-22.patch?
Comment #27
catchA year and two months latter I looked at field_default_form_errors()...
Untested patch.
Comment #29
catchComment #31
chx CreditAttribution: chx commented+ if (!empty($element['#access'])) { <= that should be if (!isset($element['#access']) || $element['#access'])
Comment #37
catchComment #38
yched CreditAttribution: yched commentedJust comparing side by side for now :
- reroll effect, patch reintroduces a t($instance['label']). t()s around labels have been removed meanwhile.
- "// Locate the correct element in the the form" : the typo is present in the current code, but let's fix it while we move the line around.
- We might want to add a line of comment above the #access check in field_default_form_errors()
Other than that, looks reasonable. We might want a test, though...
Comment #39
chipcleary CreditAttribution: chipcleary commentedsubscribe
Comment #40
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAdding "Needs tests" according to #38.
"are" will fit on the previous line.
Tabs here.
"container" will fit on the previous line.
Comment #41
xjmThe patch includes a test and its failures are exposed in the original post. Is there additional test coverage that is needed?
Comment #42
xjmTagging novice for the cleanups mentioned in #38 and #40.
Comment #43
dags CreditAttribution: dags commentedComment #44
dags CreditAttribution: dags commentedReroll patch after /core move, address #38 and #40.
Comment #46
yched CreditAttribution: yched commentedLANGUAGE_NONE has been renamed to LANGUAGE_NOT_SPECIFIED
Comment #47
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThank you, davidjdagino.
Here's a quick review:
LANGUAGE_NONE has been removed from Drupal 8 as of LANGUAGE_NONE changed to LANGUAGE_NOT_SPECIFIED, LANGUAGE_NOT_APPLICABLE and LANGUAGE_MULTIPLE added.
Note that
$langcode
is already set toLANGUAGE_NOT_SPECIFIED
in the context.Comment #48
dags CreditAttribution: dags commentedChanges from #47.
Comment #49
yched CreditAttribution: yched commentedComment #50
no_commit_credit CreditAttribution: no_commit_credit commentedAttached is identical to #48; I just wanted to make sure the tests still show the expected fails since it's been awhile since June 2010.
Comment #51
xjmAlright, those test failures look correct. All the feedback from #38 on has also been addressed. I reviewed the patch myself and found only tiny stylistic issues; the solution looks complete to me.
Extra blank line here.
This line is 81 characters.
It would be good to have assertion messages for these because otherwise we get things like "Value FALSE is FALSE" in the results which isn't so helpful. (Note: the messages should not be translated; see http://drupal.org/simpletest-tutorial-drupal7#t).
Comment #52
xjmOops, didn't mean to untag!
Comment #53
ezheidtmann CreditAttribution: ezheidtmann commentedI'm on it.
Comment #54
ezheidtmann CreditAttribution: ezheidtmann commentedFixed those style issues, added an assert message. No attempted comprehension of the rest of the patch! Thanks for the help, xjm.
Comment #55
xjmThanks @ezheidtmann! That looks good.
Comment #57
no_commit_credit CreditAttribution: no_commit_credit commentedTwo more minor tweaks: Assertion message for the other assertion, plus putting FALSE in caps per our text standards.
Comment #58
xjmRe-TBC. Thanks everyone!
Comment #59
catchThanks folks, committed pushed to 8.x, moving back to 7.x for backport.
Comment #60
catchComment #61
tim.plunkettRerolled.
Once again, I forgot that D8 uses 'complete_form' and not 'complete form' like D7.
Comment #63
tim.plunkettExcellent.
Comment #64
xjmThanks @tim.plunkett!
Comment #65
webchickThis looks like a legit bug fix, and I've no real problems backporting it, but I'd like to hold this until after Wednesday's release "just in case" it breaks something. It's possible (though unlikely) a contributed module is counting on this current situation.
Comment #66
tim.plunkettThis needs a reroll, I'll do it right afte 7.13 comes out.
Comment #67
clashar CreditAttribution: clashar commentedtim.plunkett, was it rerolled finally?
Comment #68
tim.plunkettReroll needed because of #1541792: Enable dynamic allowed list values function with additional context.
Comment #69
catchRe-roll looks good.
Comment #70
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x and added to CHANGELOG.txt - thanks! http://drupalcode.org/project/drupal.git/commit/84e34e4
Have to say I'm a little scared of this one, but code in general should be checking #access before assuming that a form element will actually be displayed... so hopefully any custom/contrib code that this breaks is code that was already somewhat broken anyway. And we have time to roll this back before the next Drupal 7 release if anyone finds something wrong with it in the meantime.
In addition to the release notes, I think this will need a D7 change notification (since it does change the array structure and the behavior of the field validation code). Moving to a critical task for that.
Comment #71
David_Rothstein CreditAttribution: David_Rothstein commentedWe probably need this change notification in place for Drupal 7.15, since I think we want to link to it from the release notes.
Comment #72
dags CreditAttribution: dags commentedComment #73
dags CreditAttribution: dags commentedAdded change notice.
Comment #74
tim.plunkettLooks good to me.
Comment #76
David_Rothstein CreditAttribution: David_Rothstein commentedThanks for the change notification (for reference, it's at http://drupal.org/node/1663020)! I've added a link to this in CHANGELOG.txt:
http://drupalcode.org/project/drupal.git/commit/4c0d034
However, this paragraph looked wrong to me:
Based on the above discussion, that was not supposed to happen in the final patch that was committed here (and I sure hope it doesn't)... I also did a quick test with the Field Permissions module (modifying the minimum allowed value of a private integer field and then trying to edit the node as a user without access to that field, per the scenario in @yched's comment above), and everything worked fine.
So, I have removed that paragraph from the change notification, and consequently also removed "Site builders, administrators, editors" from the list of affected audiences.
Obviously, if I made a mistake there somehow, it's not too late for someone to re-edit and fix it.
Comment #77
yched CreditAttribution: yched commentedSkipped the last posts there somehow. #76 is correct, and thus so is the current change notification.