You are here

Allow more Field API public API functions to act on a single field within an entity

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:

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.

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.

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 examplewe want to avoid mismatcheshence 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.

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:

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.

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.