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.

Files: 
CommentFileSizeAuthor
#68 drupal-822418-68.patch11.49 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,118 pass(es).
[ View ]
#61 drupal-822418-61-combined.patch11.29 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 38,722 pass(es).
[ View ]
#61 drupal-822418-61-tests.patch1.14 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 38,719 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#57 822418-56.patch11.34 KBno_commit_credit
PASSED: [[SimpleTest]]: [MySQL] 35,070 pass(es).
[ View ]
#57 interdiff.txt997 bytesno_commit_credit
#54 tests-only.patch1.11 KBezheidtmann
FAILED: [[SimpleTest]]: [MySQL] 35,068 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#54 complete.patch11.28 KBezheidtmann
PASSED: [[SimpleTest]]: [MySQL] 35,065 pass(es).
[ View ]
#50 drupal-822418-48-tests.patch1.05 KBno_commit_credit
FAILED: [[SimpleTest]]: [MySQL] 35,074 pass(es), 2 fail(s), and 4 exception(s).
[ View ]
#50 drupal-822418-48-complete.patch11.23 KBno_commit_credit
PASSED: [[SimpleTest]]: [MySQL] 35,072 pass(es).
[ View ]
#48 drupal-822418-48.patch11.23 KBdags
PASSED: [[SimpleTest]]: [MySQL] 35,066 pass(es).
[ View ]
#48 interdiff.txt1.81 KBdags
#44 drupal-822418-44.patch11.26 KBdags
FAILED: [[SimpleTest]]: [MySQL] 35,039 pass(es), 25 fail(s), and 32 exception(s).
[ View ]
#44 interdiff.txt2.45 KBdags
#37 form-field-acess-822418.patch10.25 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 33,641 pass(es).
[ View ]
#29 form-field-acess-822418-29.patch10.23 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 33,612 pass(es), 29 fail(s), and 0 exception(es).
[ View ]
#27 form-field-acess-822418-27.patch10.22 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 33,615 pass(es), 25 fail(s), and 19 exception(es).
[ View ]
#22 form-field-acess-822418-22.patch7.55 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 32,774 pass(es).
[ View ]
#19 form-field-acess-822418-19.patch7.52 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 32,657 pass(es), 57 fail(s), and 815 exception(es).
[ View ]
#1 822418_field_access_form.patch7.58 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 822418_field_access_form.patch.
[ View ]
field_access.patch1.19 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 20,678 pass(es), 1 fail(s), and 2 exception(es).
[ View ]

Comments

StatusFileSize
new7.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 822418_field_access_form.patch.
[ View ]

And the full patch.

Hm, 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.

Actually, 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.

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.

That 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.

"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".

Hmmm. 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"

That 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.

Priority:Normal» Major

bump. still buggy.

subscribe

subscribe

subscribe

subscribe

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

Issue tags:-needs backport to D7

#1: 822418_field_access_form.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7

The last submitted patch, 822418_field_access_form.patch, failed testing.

subscribe

StatusFileSize
new7.52 KB
FAILED: [[SimpleTest]]: [MySQL] 32,657 pass(es), 57 fail(s), and 815 exception(es).
[ View ]

Reroll of patch to head

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, form-field-acess-822418-19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.55 KB
PASSED: [[SimpleTest]]: [MySQL] 32,774 pass(es).
[ View ]

Bad reroll missed a property lets try again.

Status:Needs review» Needs work
Issue tags:-needs backport to D7

The last submitted patch, form-field-acess-822418-22.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+needs backport to D7

#22: form-field-acess-822418-22.patch queued for re-testing.

Tagging issues not yet using summary template.

To quickly fix this problem on my D7 site, can I apply form-field-acess-822418-22.patch?

StatusFileSize
new10.22 KB
FAILED: [[SimpleTest]]: [MySQL] 33,615 pass(es), 25 fail(s), and 19 exception(es).
[ View ]

A year and two months latter I looked at field_default_form_errors()...

Untested patch.

Status:Needs review» Needs work

The last submitted patch, form-field-acess-822418-27.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.23 KB
FAILED: [[SimpleTest]]: [MySQL] 33,612 pass(es), 29 fail(s), and 0 exception(es).
[ View ]

Status:Needs review» Needs work

The last submitted patch, form-field-acess-822418-29.patch, failed testing.

+ if (!empty($element['#access'])) { <= that should be if (!isset($element['#access']) || $element['#access'])

Status:Needs work» Needs review
StatusFileSize
new10.25 KB
PASSED: [[SimpleTest]]: [MySQL] 33,641 pass(es).
[ View ]

Status:Needs review» Needs work

Just 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...

subscribe

Issue tags:+Needs tests

Adding "Needs tests" according to #38.

+++ b/modules/field/field.form.incundefined
@@ -37,76 +37,73 @@ function field_default_form($entity_type, $entity, $field, $instance, $langcode,
+  // If field module handles multiple values for this form element, and we
+  // are displaying an individual element, process the multiple value form.

"are" will fit on the previous line.

+++ b/modules/field/field.form.incundefined
@@ -37,76 +37,73 @@ function field_default_form($entity_type, $entity, $field, $instance, $langcode,
+ '#field_parents' => $parents,

Tabs here.

+++ b/modules/field/field.form.incundefined
@@ -37,76 +37,73 @@ function field_default_form($entity_type, $entity, $field, $instance, $langcode,
+  // Also aid in theming of field widgets by rendering a classified
+  // container.

"container" will fit on the previous line.

Issue tags:-Needs tests

The patch includes a test and its failures are exposed in the original post. Is there additional test coverage that is needed?

Issue tags:+Novice

Tagging novice for the cleanups mentioned in #38 and #40.

Assigned:Unassigned» dags

Status:Needs work» Needs review
StatusFileSize
new2.45 KB
new11.26 KB
FAILED: [[SimpleTest]]: [MySQL] 35,039 pass(es), 25 fail(s), and 32 exception(s).
[ View ]

Reroll patch after /core move, address #38 and #40.

Status:Needs review» Needs work

The last submitted patch, drupal-822418-44.patch, failed testing.

LANGUAGE_NONE has been renamed to LANGUAGE_NOT_SPECIFIED

Thank you, davidjdagino.

Here's a quick review:

  1. Very minor: "make" would fit on the previous line.
    +++ b/core/modules/field/tests/field.testundefined
    @@ -1650,6 +1650,19 @@ class FieldFormTestCase extends FieldTestCase {
         $langcode = LANGUAGE_NOT_SPECIFIED;
  2. Minor point, too: This line appears to exceed 80 characters.
    +++ b/core/modules/field/tests/field.testundefined
    @@ -1650,6 +1650,19 @@ class FieldFormTestCase extends FieldTestCase {
    +    // Test that the form structure includes full information for each delta apart
  3. +++ b/core/modules/field/tests/field.testundefined
    @@ -1650,6 +1650,19 @@ class FieldFormTestCase extends FieldTestCase {
    +    $langcode = LANGUAGE_NONE;

    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 to LANGUAGE_NOT_SPECIFIED in the context.

StatusFileSize
new1.81 KB
new11.23 KB
PASSED: [[SimpleTest]]: [MySQL] 35,066 pass(es).
[ View ]

Changes from #47.

Status:Needs work» Needs review

StatusFileSize
new11.23 KB
PASSED: [[SimpleTest]]: [MySQL] 35,072 pass(es).
[ View ]
new1.05 KB
FAILED: [[SimpleTest]]: [MySQL] 35,074 pass(es), 2 fail(s), and 4 exception(s).
[ View ]

Attached 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.

Issue tags:-Novice

Alright, 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.

+++ b/core/modules/field/field.form.incundefined
@@ -358,31 +355,34 @@ function field_default_form_errors($entity_type, $entity, $field, $instance, $la
+      $function_exists = function_exists($function);
+
+
+      $multiple_widget = field_behaviors_widget('multiple values', $instance) != FIELD_BEHAVIOR_DEFAULT;

Extra blank line here.

+++ b/core/modules/field/field.form.incundefined
@@ -358,31 +355,34 @@ function field_default_form_errors($entity_type, $entity, $field, $instance, $la
+        // For a multiple-value widget, all errors are passed to the main widget.

This line is 81 characters.

+++ b/core/modules/field/tests/field.testundefined
@@ -1650,6 +1650,18 @@ class FieldFormTestCase extends FieldTestCase {
+    $this->assertEqual($form[$field_name_no_access][$langcode][0]['value']['#entity_type'], $entity_type);
+    $this->assertFalse($form[$field_name_no_access]['#access']);

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).

Issue tags:+Novice

Oops, didn't mean to untag!

Assigned:dags» ezheidtmann

I'm on it.

StatusFileSize
new11.28 KB
PASSED: [[SimpleTest]]: [MySQL] 35,065 pass(es).
[ View ]
new1.11 KB
FAILED: [[SimpleTest]]: [MySQL] 35,068 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Fixed those style issues, added an assert message. No attempted comprehension of the rest of the patch! Thanks for the help, xjm.

Thanks @ezheidtmann! That looks good.

StatusFileSize
new997 bytes
new11.34 KB
PASSED: [[SimpleTest]]: [MySQL] 35,070 pass(es).
[ View ]

Two more minor tweaks: Assertion message for the other assertion, plus putting FALSE in caps per our text standards.

Status:Needs review» Reviewed & tested by the community

Re-TBC. Thanks everyone!

Status:Reviewed & tested by the community» Patch (to be ported)

Thanks folks, committed pushed to 8.x, moving back to 7.x for backport.

Version:8.x-dev» 7.x-dev

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.14 KB
FAILED: [[SimpleTest]]: [MySQL] 38,719 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
new11.29 KB
PASSED: [[SimpleTest]]: [MySQL] 38,722 pass(es).
[ View ]

Rerolled.

Once again, I forgot that D8 uses 'complete_form' and not 'complete form' like D7.

Status:Needs review» Needs work

The last submitted patch, drupal-822418-61-tests.patch, failed testing.

Status:Needs work» Needs review

Excellent.

Status:Needs review» Reviewed & tested by the community

Thanks @tim.plunkett!

This 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.

Assigned:ezheidtmann» tim.plunkett
Status:Reviewed & tested by the community» Needs work

This needs a reroll, I'll do it right afte 7.13 comes out.

tim.plunkett, was it rerolled finally?

Status:Needs work» Needs review
StatusFileSize
new11.49 KB
PASSED: [[SimpleTest]]: [MySQL] 39,118 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Re-roll looks good.

Title:Field form structure incomplete if field_access() returns FALSEChange notification for: Field form structure incomplete if field_access() returns FALSE
Category:bug» task
Priority:Major» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+7.15 release notes

Committed 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.

Issue tags:+7.15 release blocker

We probably need this change notification in place for Drupal 7.15, since I think we want to link to it from the release notes.

Assigned:tim.plunkett» dags

Status:Active» Needs review

Added change notice.

Title:Change notification for: Field form structure incomplete if field_access() returns FALSEField form structure incomplete if field_access() returns FALSE
Category:task» bug
Priority:Critical» Major
Status:Needs review» Fixed

Looks good to me.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Thanks 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:

With this change, site administrators should be particularly careful about modifying the allowed values of existing fields. If a field's existing value lies outside the bounds of allowed values, and a user does not have access to edit that field, then the user can not edit the value to make it valid and will therefore not be allowed to save any changes on the entire form.

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.

Skipped the last posts there somehow. #76 is correct, and thus so is the current change notification.