Problem/Motivation

The FAPE and Edit modules need to be able to build, validate, submit single field forms, as well as render single fields.

The former is most definitely not directly supported by Field API (hence the need for FAPE to duplicate a lot of code, and Edit piggybacks on FAPE). The latter is, but only partially: field_view_field() allows you to do this, but it doesn't allow the caller to set a view mode; instead it creates its own view mode: '_custom'.

Proposed resolution

The internal field_invoke_method() already supports this, through the $options parameter. All I'm proposing is to allow public API functions to also use pass this parameter.

It's a trivial patch :)

Remaining tasks

None, besides preferably a backport to D7.

User interface changes

None.

API changes

The following functions all receive a new, trailing, optional $options parameter:

  • field_attach_form()
  • field_attach_validate()
  • field_attach_form_validate()
  • field_attach_submit()
  • field_attach_prepare_view()
  • field_attach_view()
Files: 
CommentFileSizeAuthor
#45 field-api-single-field-1821906-45-followup.patch3.17 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,266 pass(es).
[ View ]
#41 field-api-single-field-1821906-41-followup.patch7.11 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,344 pass(es).
[ View ]
#37 core-field_api_single_field-1821906-37.patch22.78 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 40,241 pass(es).
[ View ]
#28 core-field_api_single_field-1821906-28.patch22.81 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 39,828 pass(es).
[ View ]
#28 interdiff.txt809 bytesWim Leers
#26 core-field_api_single_field-1821906-26.patch22.53 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 39,796 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#24 core-field_api_single_field-1821906-24.patch22.53 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,025 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#21 core-field_api_single_field-1821906-21.patch22.54 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 39,670 pass(es), 32 fail(s), and 32 exception(s).
[ View ]
#15 interdiff.txt2.69 KBWim Leers
#15 field_api_single_field-1821906-15.patch23.1 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 46,287 pass(es).
[ View ]
#13 field_api_single_field-1821906-13.patch23.06 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 46,280 pass(es).
[ View ]
#12 interdiff.txt3.69 KBWim Leers
#12 field_api_single_field-1821906-12.patch3.69 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_api_single_field-1821906-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 interdiff.txt5.26 KBWim Leers
#9 field_api_single_field-1821906-9.patch23.55 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 46,285 pass(es).
[ View ]
#5 interdiff.txt17.98 KBWim Leers
#5 field_api_single_field-1821906-5.patch23.41 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 46,272 pass(es).
[ View ]
#1 field_api_single_field-1821906-1.patch6.45 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 46,236 pass(es).
[ View ]

Comments

Status:Active» Needs review
Issue tags:+Spark
StatusFileSize
new6.45 KB
PASSED: [[SimpleTest]]: [MySQL] 46,236 pass(es).
[ View ]

Title:Allow more Field API public API functions to act on a single fieldAllow more Field API public API functions to act on a single field within an entity
Status:Needs review» Needs work
Issue tags:+Needs tests

The original title was impossible, but this, I can support this.

Also, if at possible do not call field_attach_* directly, use the entity API. Is that possible?

AFAICT that is unfortunately impossible. AFAICT Entity API will always generate forms for the entire entity, nor does it allow for rendering individual fields of an entity.

Status:Needs work» Needs review
StatusFileSize
new23.41 KB
PASSED: [[SimpleTest]]: [MySQL] 46,272 pass(es).
[ View ]
new17.98 KB

Found a bug thanks to tests :)

It ain't pretty, but then again the current test aren't pretty either. The scope of the initial tests was rather limited: it tested an entity with a single field. I've now expanded that to test an entity with 2 fields. So now it is also tested whether the functions work correctly for multiple fields at once.
For each of the tests, I then test whether the $options = array('field_name' => $field_name) parameter is obeyed: I always feed the necessary data to view/generate a form for/validate/submit for multiple fields (identical to the "default" case as explained above), but now it should only perform the asked actions on a single field.

Issue tags:+Entity system

Trying this tag to get some more eyeballs on this. I know it's field API and not entity API, but the same sorts of people are working on both. :)

Status:Needs review» Reviewed & tested by the community

Go. Does not add more ugly than what's in the tests already which are quite ugly TBH.

Category:feature» task
Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldAttachTestBase.phpundefined
@@ -43,4 +43,30 @@ function setUp() {
+  function setUpSecondField() {

This needs docblock. Typically though, we don't name functions like setUpBlahBlah, but instead createBlahBlah. It's a little funny.

This also looks like largely a copy/paste of FieldTestBase::setUp(), so instead let's take the code out of there, wrap it in a function called createField() and call it from both places. (and docblock it :D)

Also, in talking to xjm she felt this was more a task than a feature, so re-classifying.

Status:Needs work» Needs review
StatusFileSize
new23.55 KB
PASSED: [[SimpleTest]]: [MySQL] 46,285 pass(es).
[ View ]
new5.26 KB

Done.

I find that $suffix stuff a bit weird. I talked to Wim about it and he said:

it's not sufficient, it could cause random matches
"field_label" is a subset of "field_label_2" for example
we want to avoid mismatches
hence the need for this trickery :)
otherwise you have randomly failing tests… :)

On the surface this seems sound, but I talked it over with xjm and we both don't see how there's any less than a 1 in 24 hobityjillion chance of this happening, and it makes this function kinda weird and non-standard compared to other createX() ones that tests use.

xjm's recommended changes:

1) drupalCreateNode() should be our model here, where it allows you to pass in explicit $settings for properties (if it's actually material to the test to have a suffix on the field, which I don't really see), else it just sets them to reasonable defaults.

2) She also recommended moving that helper function to WebTestBase instead, so other tests can make use of it.

3) She further mentioned that at one point or another David Rothstein had recommended against a helper function in favour of in-lining field_create_instance() calls, but there seems to be enough code in FieldAttachTestBase::setUp to warrant the function, to me.

Sorry, Wim. :( I know this code is only tangential to your actual proposed change, but testing is a gate.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new3.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_api_single_field-1821906-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.69 KB

On the surface this seems sound, but I talked it over with xjm and we both don't see how there's any less than a 1 in 24 hobityjillion chance of this happening, and it makes this function kinda weird and non-standard compared to other createX() ones that tests use.

The test verify whether things work correctly by checking for label presence, value presence etc. Values are always in the [1,127] range. I had a collision on those. But I actually prevented collisions in a more effective manner already:

-      $this->assertNoRaw($value['value'], "Hidden field: value $delta is not displayed.");
+      $this->assertNoRaw("$formatter_setting|{$value['value']}", "Hidden field: value $delta is not displayed.");

So I dropped the $label_suffix parameter (at the cost of making it harder to debug/extend tests), but I cannot drop the $suffix parameter, because otherwise it becomes impossible to refer to the different aspects of the field instance. $suffix is used to create $this->$field_name for example, where $field_name is 'field_name' . $suffix. I need to be able to point to $code->field_name, $this->field_name_2, etc. to be able to do the tests at all.

1) drupalCreateNode() should be our model here, where it allows you to pass in explicit $settings for properties (if it's actually material to the test to have a suffix on the field, which I don't really see), else it just sets them to reasonable defaults.
2) She also recommended moving that helper function to WebTestBase instead, so other tests can make use of it.

This doesn't make any sense; this is of zero use to general tests, it's only useful for testing correct functioning of Field API's attach logic. Hence it lives in FieldAttachTestBase, and should continue to live there IMO.
Creating something like drupalCreateNode() is equally pointless, since the point is that we don't have to control specific settings, but rather, they have randomized values so that no field name, field label, field description and whatnot is ever the same. If we go with the drupalCreateNode() model, then it won't be a simple one-liner to createFieldInstance() as it is right now, then it would be a massive $settings array with a boatload of $this->randomName() calls.

Sorry, Wim. :( I know this code is only tangential to your actual proposed change, but testing is a gate.

And I'm glad it is a gate! :)

So, overall I do understand your concern and now I've reduced it to a minimum. But *this* is what makes sense though.

StatusFileSize
new23.06 KB
PASSED: [[SimpleTest]]: [MySQL] 46,280 pass(es).
[ View ]

Ugh, the patch is borked, it's just another interdiff. Here's the right one.

Status:Needs review» Needs work

Looks fine to me. I like the trickery in the helper method, one nitpick before RTBC though:

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldAttachTestBase.phpundefined
@@ -19,12 +19,28 @@
+  /**
+   * Create a field instance.
+   *
+   * @param string $suffix
+   *   (optional) A string that should only contain characters that are valid in
+   *   PHP variable names as well.
+   */
+  function createFieldInstance($suffix = '') {

The documentation is a bit confusing as the method will create a field AND an instance, so I'd change that and rename the method to createFieldWithInstance()

Status:Needs work» Needs review
StatusFileSize
new23.1 KB
PASSED: [[SimpleTest]]: [MySQL] 46,287 pass(es).
[ View ]
new2.69 KB

Done :)

Status:Needs review» Reviewed & tested by the community

Let's do this! For people wanting more background, we need this for #1824500: In-place editing for Fields, pure awesomeness.

Title:Allow more Field API public API functions to act on a single field within an entityChange notice: Allow more Field API public API functions to act on a single field within an entity
Priority:Minor» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Ok, thanks swentel!

Committed and pushed to 8.x.

Let's get a change notice for this.

WOOT! Thanks, @swentel! :)

Change record created: http://drupal.org/node/1825844.

Now one question remains: would it be also acceptable to backport this to Drupal 7? It'd make the lives of FAPE and Edit in D7 infinitely easier, and it doesn't break any existing API. So it definitely is feasible.

Title:Change notice: Allow more Field API public API functions to act on a single field within an entityAllow more Field API public API functions to act on a single field within an entity

.

Version:8.x-dev» 7.x-dev
Priority:Critical» Normal
Status:Active» Patch (to be ported)
Issue tags:+needs backport to D7

Change notice exists, reducing down to a regular task.

I think it's worth a shot. It doesn't seem like it would break anything, since it's just an optional argument. Moving to D7.

Status:Patch (to be ported)» Needs review
StatusFileSize
new22.54 KB
FAILED: [[SimpleTest]]: [MySQL] 39,670 pass(es), 32 fail(s), and 32 exception(s).
[ View ]

Ok let's try that

Status:Needs review» Needs work

The last submitted patch, core-field_api_single_field-1821906-21.patch, failed testing.

s/LANGUAGE_NOT_SPECIFIED/'und'/ should fix (most of) the fails.

Status:Needs work» Needs review
StatusFileSize
new22.53 KB
FAILED: [[SimpleTest]]: [MySQL] 40,025 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, core-field_api_single_field-1821906-24.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new22.53 KB
FAILED: [[SimpleTest]]: [MySQL] 39,796 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Using the right name for a function helps… Ok, only one fail now, not sure why yet though. uploading in case someone finds the issue before me.

Status:Needs review» Needs work

The last submitted patch, core-field_api_single_field-1821906-26.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new809 bytes
new22.81 KB
PASSED: [[SimpleTest]]: [MySQL] 39,828 pass(es).
[ View ]

This should do the trick. You forgot to pass $options in one place… :)

Assigned:Wim Leers» yched
Status:Needs review» Reviewed & tested by the community

Checked and looks good to me and doesn't conflict (git apply wise) with #1040790: _field_info_collate_fields() memory usage. Will probably need a change notices as well for D7 as it has API additions.

Hmm, didn't want to necessarily assign it to yched, but maybe it can't hurt anyway :)

Assigned:yched» Unassigned

Works for me :-)

looks like http://drupal.org/node/1825844 created earlier fits. just need to add the 7.x branch and that should be good?

@nod_ should be probably be fine yes :)

...coming to this issue because of the need to apply the latest patch in order to get edit to work in D7 prior to 7.20 - so following ;)

Bump… can we get this committed please? :)

Status:Reviewed & tested by the community» Needs work

I don't see the field_invoke_method() in D7.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new22.78 KB
PASSED: [[SimpleTest]]: [MySQL] 40,241 pass(es).
[ View ]

s/field_invoke_method()/_field_invoke()/ then.

Status:Reviewed & tested by the community» Fixed
Issue tags:+7.22 release notes

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/b9d7b6f

I updated the change notification at http://drupal.org/node/1825844 to refer to 7.x as well and linked to it in the D7 CHANGELOG.txt.

I did notice a couple small things for followups (but they are both minor). I believe at least the first one affects Drupal 8 too.

  1. + * @param array $options
    + *   An associative array of additional options. See _field_invoke() for
    + *   details.
      */
    -function field_attach_validate($entity_type, $entity) {
    +function field_attach_validate($entity_type, $entity, $options = array()) {

    This @param is being added in the wrong place (there is a @throws right above it that should be underneath instead).

  2.    function testFieldAttachForm() {
    ......
    -      $this->assertEqual($form[$this->field_name][$langcode][$delta]['value']['#type'], 'textfield', "Form delta $delta widget is textfield");
    -    }
    +        $this->assertEqual($form[$this->field_name][$langcode][$delta]['value']['#type'], 'textfield', "First field's form delta $delta widget is textfield");
    +      }

    (and several lines of code below it also) - the indentation is incorrect.

I have used this patch + Bean module and I get an error:

Fatal error: Unsupported operand types in /.../modules/field/field.attach.inc on line 188

My report of the issue in the Bean project: #1957260: Fatal error: Unsupported operand types

Status:Fixed» Needs work
Issue tags:+7.22 release blocker

Thanks for finding that.

This happens because bean_form_validate() in the Bean module calls this:

<?php
  field_attach_form_validate
('bean', $bean, $form, $form_state, $langcode);
?>

But there is no $langcode parameter in field_attach_form_validate().

However, the patch here certainly turned that from a harmless bug into a fatal error (since there is now a new $options parameter at the end which expects an array as input).

So I think we should do something to resolve that. One possibility is to validate the input $options as an array and only use it if it is...

Status:Needs work» Needs review
StatusFileSize
new7.11 KB
PASSED: [[SimpleTest]]: [MySQL] 40,344 pass(es).
[ View ]

Something like this, for example.

Doesn't do anything to help if they happen to be passing in some other array by mistake, but I guess the chances of that causing a meaningful problem are very low.

Otherwise, it should help, and seems to fix the issue with Bean.

We probably need to resolve this one way or another in the next day or so (either go with this approach or roll back the original commit for now) since I'm hoping to release Drupal 7.22 on Wednesday.

Hm. That seems like an awful lot of hand-holding.

Is it possible instead to cast it to an array?

This is a bug in the Bean module, the field_attach_form_validate function didn't have this fifth argument in the first place.

Is it possible instead to cast it to an array?

That would probably work - does run a slightly higher risk of data winding up in the array that wasn't supposed to be there, though.

Either way I suppose there is no need to assign a new variable. It could be $options = is_array($options) ? $options : array() instead.

StatusFileSize
new3.17 KB
PASSED: [[SimpleTest]]: [MySQL] 40,266 pass(es).
[ View ]

Like this, for example.

Status:Needs review» Reviewed & tested by the community

Much better. :)

I think that looks like a reasonable way to go about this, and would hopefully prevent the Bean error and others like it lurking around.

Status:Reviewed & tested by the community» Fixed
Issue tags:-7.22 release blocker

OK, thanks, committed to 7.x - http://drupalcode.org/project/drupal.git/commit/9c52b71

Hopefully we're all set here.

Thanks David! :)

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

Issue summary:View changes

Updated issue summary.