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.

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.

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

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

subscribe

Bad reroll missed a property lets try again.

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

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

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

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

subscribe

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

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

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.

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.

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

I'm on it.

Thanks @ezheidtmann! That looks good.

Re-TBC. Thanks everyone!

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

Thanks @tim.plunkett!

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

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.

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.