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()
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
Issue tags: +Spark
FileSize
6.45 KB
chx’s picture

Title: Allow more Field API public API functions to act on a single field » Allow 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.

chx’s picture

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

Wim Leers’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
23.41 KB
17.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.

webchick’s picture

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

chx’s picture

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.

webchick’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
23.55 KB
5.26 KB

Done.

webchick’s picture

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.

webchick’s picture

Status: Needs review » Needs work
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.69 KB
3.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.

Wim Leers’s picture

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

swentel’s picture

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
23.1 KB
2.69 KB

Done :)

swentel’s picture

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.

webchick’s picture

Title: Allow more Field API public API functions to act on a single field within an entity » Change 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.

Wim Leers’s picture

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.

Wim Leers’s picture

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

.

webchick’s picture

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.

nod_’s picture

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

Ok let's try that

Status: Needs review » Needs work

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

Wim Leers’s picture

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

nod_’s picture

Status: Needs work » Needs review
FileSize
22.53 KB

Status: Needs review » Needs work

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

nod_’s picture

Status: Needs work » Needs review
FileSize
22.53 KB

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
809 bytes
22.81 KB

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

swentel’s picture

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.

swentel’s picture

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

yched’s picture

Assigned: yched » Unassigned

Works for me :-)

nod_’s picture

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

swentel’s picture

@nod_ should be probably be fine yes :)

klonos’s picture

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

Wim Leers’s picture

Bump… can we get this committed please? :)

jcisio’s picture

Status: Reviewed & tested by the community » Needs work

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

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
22.78 KB

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

David_Rothstein’s picture

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.

Iztok’s picture

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

David_Rothstein’s picture

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:

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

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
7.11 KB

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.

webchick’s picture

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

Is it possible instead to cast it to an array?

DamienMcKenna’s picture

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

David_Rothstein’s picture

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.

David_Rothstein’s picture

Like this, for example.

webchick’s picture

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.

David_Rothstein’s picture

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.

Wim Leers’s picture

Thanks David! :)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture