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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

And the full patch.

yched’s picture

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.

yched’s picture

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.

catch’s picture

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.

yched’s picture

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

moshe weitzman’s picture

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"

catch’s picture

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.

chx’s picture

Priority: Normal » Major

bump. still buggy.

bryancasler’s picture

subscribe

gagoo’s picture

subscribe

clashar’s picture

ray17n’s picture

subscribe

restyler’s picture

tobey_p’s picture

subscribe

sun’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
sun’s picture

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.

snupy’s picture

subscribe

marcingy’s picture

Reroll of patch to head

marcingy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

marcingy’s picture

Status: Needs work » Needs review
FileSize
7.55 KB

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.

marcingy’s picture

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

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

xjm’s picture

Tagging issues not yet using summary template.

Arla’s picture

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

catch’s picture

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.

catch’s picture

Status: Needs work » Needs review
FileSize
10.23 KB

Status: Needs review » Needs work

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

chx’s picture

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

catch’s picture

Status: Needs work » Needs review
FileSize
10.25 KB
yched’s picture

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

chipcleary’s picture

subscribe

Niklas Fiekas’s picture

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.

xjm’s picture

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?

xjm’s picture

Issue tags: +Novice

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

dags’s picture

Assigned: Unassigned » dags
dags’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
11.26 KB

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

Status: Needs review » Needs work

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

yched’s picture

LANGUAGE_NONE has been renamed to LANGUAGE_NOT_SPECIFIED

Niklas Fiekas’s picture

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.

dags’s picture

FileSize
1.81 KB
11.23 KB

Changes from #47.

yched’s picture

Status: Needs work » Needs review
no_commit_credit’s picture

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.

xjm’s picture

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

xjm’s picture

Issue tags: +Novice

Oops, didn't mean to untag!

ezheidtmann’s picture

Assigned: dags » ezheidtmann

I'm on it.

ezheidtmann’s picture

FileSize
11.28 KB
1.11 KB

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

xjm’s picture

Thanks @ezheidtmann! That looks good.

no_commit_credit’s picture

FileSize
997 bytes
11.34 KB

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

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Re-TBC. Thanks everyone!

catch’s picture

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

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

catch’s picture

Version: 8.x-dev » 7.x-dev
tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.14 KB
11.29 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review

Excellent.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @tim.plunkett!

webchick’s picture

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.

tim.plunkett’s picture

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.

clashar’s picture

tim.plunkett, was it rerolled finally?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.49 KB
catch’s picture

Status: Needs review » Reviewed & tested by the community

Re-roll looks good.

David_Rothstein’s picture

Title: Field form structure incomplete if field_access() returns FALSE » Change 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.

David_Rothstein’s picture

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.

dags’s picture

Assigned: tim.plunkett » dags
dags’s picture

Status: Active » Needs review

Added change notice.

tim.plunkett’s picture

Title: Change notification for: Field form structure incomplete if field_access() returns FALSE » Field 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.

David_Rothstein’s picture

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.

yched’s picture

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