Spin-off from #1499596: Introduce a basic entity form controller: we need to be able to reuse the form generated by an entity form controller and ensure it can be embedded into another form.

To be completed.

#18 #drupal-entity-21-02-14.txt4.2 KByched


fago’s picture

and we need to make sure that we can embed multiple forms at the same time!

sun’s picture

effulgentsia’s picture

I'm interpreting this issue as wanting to put the $form returned by EntityFormControllerInterface::build() into some other $form such that the HTML output is a single <form> tag. Is that correct, or is this about wanting multiple $form arrays sharing the same $form_id (e.g., several entities of the same type) to be each rendered as their own <form> tags, but to not have id / form processing collisions when all rendered on the same page?

EclipseGc’s picture

fwiw, I'm interpreting this as meaning that I could put any (or many) entity forms into the same form and determine what sort of entity forms existed within this and hand off processing of that separately. Something like:

= array();
$node = entity_create('node', array(
'uid' => $user->uid,
'type' => 'page',
'langcode' => node_type_get_default_langcode('page'),
$form['node'] = entity_get_form($node);
$form['something_else'] = array(
'#type' => 'select',
'#options' => ...

And then be able to iteratively pass the form state to entity_form_submit and have it process the various entity forms embedded in the greater form. Is this a correct interpretation?


plach’s picture

I think @fago's plan is to support both scenarios in #3.


Is this correct?


I think your example is not correct because entity_get_form() returns an already processed form ready to be rendered. As pointed out in #3 the correct way should be using the value returned by EntityFormControllerInterface::build().

fago’s picture

I was thinking of what is described in #4. Avoinding form id collisions is something different we might want to fix generally elsewhere.

andypost’s picture

Suppose menu_overview_form is nice example of 2 forms

yched’s picture

I missed that issue so far, interesting, it's precisely the kind of things I'm about to bump into in #1992138: Helper issue for "field types as TypedData Plugins".

Late in the D7 cycle, we had quite some work to make sure Field API form integration (include widgets, extract values, validate and assign errors back to form elements) works with forms containing several entities. field_test_entity_nested_form() is a simple example in core right now, and IIRC, field_collection relies on this mechanism.

Not sure how this can play with EntityFormControllers that include more and more entity logic. #1992138: Helper issue for "field types as TypedData Plugins", among other things, removes field_attach_form_validate(), and moves the equivalent code in the EFC - and right now I'm not sure how to make the tests around field_test_entity_nested_form() pass again.

Berdir’s picture

Issue tags:+Entity Field API


Xano’s picture

fago’s picture

Priority:Normal» Major
Issue summary:View changes
Issue tags:+beta target

Given entity forms need to replace field_attach_form() we need to fix this for entity forms being a full replacement.

yched’s picture

Discussion in #2090509: Optimize entity_get_render_display() for the case of "multiple entity view" derived on "Edit in place"'s EditFieldForm, and from here "inline entity form" - see #2090509-28: Optimize entity_get_render_display() for the case of "multiple entity view".

The current situation is :
- Base fields can now be rendered by widgets.
- The ones that aren't are rendered through custom code (either in EntityFormController:buildForm() or in form alters) that is only triggered by "the official entity form with the right form controller and form_id"
- It's difficult to abstract those out and decide whether they should also be applied to an inlined form or a "single field" form.

Wouldn't it just be good enough to provide easy support for "include field widgets for an entity in your custom form/subform" (with validation and extraction of values) ? - i.e simply our current field_attach_form*() refreshed for OO ?
Does "inline entity form" do anything else than rendering the field widgets right now ?

yched’s picture

#2095195: Remove deprecated field_attach_form_*() has an initial patch with those "generic widget attachers". The question is: where do we put them...

Berdir’s picture

I don't know what else is left to do here?

NestedEntityTestForm is an example and test coverage that we can embed/edit multiple entities in the same form.

I know that profile2 is successfully using a similar approach (based on that example), the display profile edit forms within the user registration form.

yched’s picture

To clarify :
We made sure is still possible is to attach the (widgets for the) fields of various entities at various places within one form. This is what NestedEntityTestForm tests (should probably be renamed...). In D7 terms, you can call field_attach_form() for various entities in various places in your form.

Granted, in D8, that gives you much more than in D7, but that's still limited to "the generic field / widget behavior".
You can edit the fields of two entities in your form, but the "subforms" won't leverage any of the logic that is present in the EntityForm / ContentEntityForm / [MyEntityType]Form classes. And that logic is currently a very intricated mix of a) entity massaging stuff that is relevent whenever an entity is created / editied in a form and b) UI-like stuff that is really related to "this form being the top-level form" (redirects, dsm()s...)

As a side note, NestedEntityTestForm currently only works with two pre-existing entities. If it wanted to allow creation of new entities (which is not relevant to its testing scope), it would have to do some additional legwork of its own.

This being said :
I happened to bump into @bojanz (who maintains Inline Entity Form) recently, and talked with him about this, he said at worst he still planned to have EntityType-specific handler classes for integration with Inline Entity Form.

So basically, the situation is not worse that in D7. Entity forms are not nestable, integrating your entity type with Inline Entity Form requires you to provide some specific integration class. There will be a separate InlineEntityFormController / [MyEntityType]InlineFormController stack that duplicates some of what's done in EntityForm / ContentEntityForm / [MyEntityType]Form for "main forms". Not ideal, but not worse than D7, core just won't have allowed progress on that side, we'll live...

In short : yeah, probably a won't fix...

fago’s picture

Given #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title) the question is whether tacking this issue is still required / needed. Given most stuff can move into the entity display form which is already embeddable, it should be enough to have only entity display forms as embeddable entity forms.

Then, the full entity forms would be just he conrete incarnation of the entity form display, whereas subforms somewhere else would be different incarnations. Given that it makes sense to have different form alters, i.e. having the main entity form alter not in the sub-form. The right way for modules to add something would be widgets anyway.

Then, I guess the question remains whether we need to have a generic alter accross forms generated from entity form displays. Given we had hook_field_attach_form() it even seems to be a regression to not have it right now, or do we?

However, for #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title) to work out nicely we have to solve it's problematic areas. Discussing it with Wimleers and berdir, it seems it boils down to mostly
- groups
- help texts
- other non-field stuff which needs a widget like revision logs

Thus, we'd need #1875974: Abstract 'component type' specific code out of EntityDisplay solved - what I'd assume fieldgroup needs anwayway.

Given that, I think it's better to focus on moving more stuff into display and have that as re-usable, embeddable entity form only. Does that seem feasible for inline entity form?

yched’s picture

Given we had hook_field_attach_form() it even seems to be a regression to not have it right now, or do we?

Digging in #2095195: Remove deprecated field_attach_form_*() :

- @yched in #79:

hook_field_attach_form() allows altering a $form array containing all the widgets for the entity
Same as in #2167267: Remove deprecated field_attach_*_view(), it doesn't duplicate hook_[entity]_form_alter() because that one only runs on "full entity forms" (EntityFormController), not for nested forms.

But we already have hook_field_widget_form_alter(), which runs for each widget individually (there's no equivalent for formatters for performance reasons).
So hook_field_attach_form() only adds the ability for "cross widget" manipulation ?

- @fago in #79-2:

While this seems reasonable, what's the use-case one would go with this hook compared to the entity form hook?
If it's just about having a generic hook, I guess it tells us we miss a generic entity form hook instead

- @yched in #80:

Yes, last thursday on IRC it was decided to remove those hooks, but that was not done yet. Done now

(and the comment's interdiff patch removes the "@todo what about hook_field_attach_form()")

- @tim.plunkett in #108 (post commit)

Ouch, I missed that this removed hook_field_attach_form(). That will hurt, as it was a nice pairing for hook_field_extra_fields() in D7

So, according to #80, we concluded in an IRC meeting (probably on feb. 20th 2014 ?) that we would just ditch hook_field_attach_form(). IIRC, it was more like "no clear use case atm, let's remove it for now and revisit if needed".

@Berdir, @amateescu, rings a bell ?

yched’s picture

new4.2 KB

Looking through my logs (feb. 21st 2014, actually), that's exactly what happened - "no clear case, we'll see in #1728816: Ensure multiple entity forms can be embedded into one form" :-)

Attaching the relevant part of the discussion.

yched’s picture

Other than that, yes, the more processing gets handled at the widget / entity form display level, the less custom code to write in dedicated IEF form controllers...

yched’s picture

@Wim Leers just wrote this in #2226493-145: Apply formatters and widgets to Node base fields :

since #2098355: Missing default access for all node fields landed, we should be able to remove the #access stuff in NodeForm; which means almost everything of the node form will be generated!


fago’s picture

Yeah, we figured #access handling is taken care of by entity field access, for which we've to do #2028027: [META] Missing access control for base fields anyway.

bojanz’s picture

That's great progress, everyone!

I can confirm #19.
Thank you yched for fighting for my pet module :)