Field form structure incomplete if field_access() returns FALSE

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.

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

bump. still buggy.

subscribe

subscribe

subscribe, came from #1062072: Notice : Undefined property: stdClass::$field_name_of_the_field in locale_field_node_form_submit()

subscribe

subscribing from #1062072: Notice : Undefined property: stdClass::$field_name_of_the_field in locale_field_node_form_submit()

subscribe

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

subscribe

Reroll of patch to head

Bad reroll missed a property lets try again.

#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?

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

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

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.

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

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

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

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.

Changes from #47.

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.

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

Oops, didn't mean to untag!

I'm on it.

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.

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

Re-TBC. Thanks everyone!

Rerolled.

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

Excellent.

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.

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

tim.plunkett, was it rerolled finally?

Reroll needed because of #1541792: Enable dynamic allowed list values function with additional context.

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.

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.

Added change notice.

Looks good to me.

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.