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()
Comment | File | Size | Author |
---|---|---|---|
#45 | field-api-single-field-1821906-45-followup.patch | 3.17 KB | David_Rothstein |
#41 | field-api-single-field-1821906-41-followup.patch | 7.11 KB | David_Rothstein |
#37 | core-field_api_single_field-1821906-37.patch | 22.78 KB | Wim Leers |
#28 | core-field_api_single_field-1821906-28.patch | 22.81 KB | Wim Leers |
#28 | interdiff.txt | 809 bytes | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
chx CreditAttribution: chx commentedThe original title was impossible, but this, I can support this.
Comment #3
chx CreditAttribution: chx commentedAlso, if at possible do not call field_attach_* directly, use the entity API. Is that possible?
Comment #4
Wim LeersAFAICT 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.
Comment #5
Wim LeersFound 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.Comment #6
webchickTrying 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. :)
Comment #7
chx CreditAttribution: chx commentedGo. Does not add more ugly than what's in the tests already which are quite ugly TBH.
Comment #8
webchickThis 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.
Comment #9
Wim LeersDone.
Comment #10
webchickI find that $suffix stuff a bit weird. I talked to Wim about it and he said:
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.
Comment #11
webchickComment #12
Wim LeersThe 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.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.
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.
Comment #13
Wim LeersUgh, the patch is borked, it's just another interdiff. Here's the right one.
Comment #14
swentel CreditAttribution: swentel commentedLooks fine to me. I like the trickery in the helper method, one nitpick before RTBC though:
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()
Comment #15
Wim LeersDone :)
Comment #16
swentel CreditAttribution: swentel commentedLet's do this! For people wanting more background, we need this for #1824500: In-place editing for Fields, pure awesomeness.
Comment #17
webchickOk, thanks swentel!
Committed and pushed to 8.x.
Let's get a change notice for this.
Comment #18
Wim LeersWOOT! 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.
Comment #19
Wim Leers.
Comment #20
webchickChange 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.
Comment #21
nod_Ok let's try that
Comment #23
Wim Leerss/
LANGUAGE_NOT_SPECIFIED
/'und'
/ should fix (most of) the fails.Comment #24
nod_Comment #26
nod_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.
Comment #28
Wim LeersThis should do the trick. You forgot to pass
$options
in one place… :)Comment #29
swentel CreditAttribution: swentel commentedChecked 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.
Comment #30
swentel CreditAttribution: swentel commentedHmm, didn't want to necessarily assign it to yched, but maybe it can't hurt anyway :)
Comment #31
yched CreditAttribution: yched commentedWorks for me :-)
Comment #32
nod_looks like http://drupal.org/node/1825844 created earlier fits. just need to add the 7.x branch and that should be good?
Comment #33
swentel CreditAttribution: swentel commented@nod_ should be probably be fine yes :)
Comment #34
klonos...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 ;)
Comment #35
Wim LeersBump… can we get this committed please? :)
Comment #36
jcisio CreditAttribution: jcisio commentedI don't see the field_invoke_method() in D7.
Comment #37
Wim Leerss/
field_invoke_method()
/_field_invoke()
/ then.Comment #38
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted 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.
This @param is being added in the wrong place (there is a @throws right above it that should be underneath instead).
(and several lines of code below it also) - the indentation is incorrect.
Comment #39
Iztok CreditAttribution: Iztok commentedI 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
Comment #40
David_Rothstein CreditAttribution: David_Rothstein commentedThanks for finding that.
This happens because bean_form_validate() in the Bean module calls this:
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...
Comment #41
David_Rothstein CreditAttribution: David_Rothstein commentedSomething 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.
Comment #42
webchickHm. That seems like an awful lot of hand-holding.
Is it possible instead to cast it to an array?
Comment #43
DamienMcKennaThis is a bug in the Bean module, the field_attach_form_validate function didn't have this fifth argument in the first place.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commentedThat 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.Comment #45
David_Rothstein CreditAttribution: David_Rothstein commentedLike this, for example.
Comment #46
webchickMuch 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.
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commentedOK, thanks, committed to 7.x - http://drupalcode.org/project/drupal.git/commit/9c52b71
Hopefully we're all set here.
Comment #48
Wim LeersThanks David! :)
Comment #49.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #50
sunFollow-up: #2306539: FieldUnitTestBase::createFieldWithInstance() pollutes test class with arbitrary properties