Add an EmbeddableFormInterface so that FormInterface can delegate to multiple objects

Basically +1000000000 to this. I think we want to bring the various plugins in line here. Blocks is a really big offender here doing it's own thing. I think actions does something similar in the patches tim has in the queue, and Conditions outright implements FormInterface. We need to be able to say a plugin is expected to render a form in the UI, and allow the wrapping code the provide the presentation of that form as well as the buttons to submit/cancel/etc it.

Eclipse

I would need this as well for "entity subforms" (or inline entity forms).

Possibly consider a better name than SubFormInterface.

How about FormFragmentInterface?

+1 to idea, that would be helpful for menu links and path elements. And nice example is menu_overview_form()

About naming - Fragment is a part of smth but here we dealing with form a whole but Embeddable into other form

ok, yeah I like EmbeddableFormInterface.

Eclipse

I totally see the need for something like this, however this solution won't work for entity forms. See #1728816: Ensure multiple entity forms can be embedded into one form.

While I think this is a reasonable and straight-forward approach, I wonder whether it makes sense once we need something entities as well? So maybe we should just make entities fit into this approach also... the difficult point being there is altering.

+++ b/core/lib/Drupal/Core/Form/EmbeddableFormInterface.php
@@ -0,0 +1,50 @@
+  public function buildEmbeddedForm(array $form, array &$form_state);

This method names seem to be quite a bit verbose. Couldn't we just go with validateForm() as usual?

+1 on something like this being needed so that EntityFormControllers can by used at several places in a form, to support "several entities in a form". This is pretty important since that's what field_collection relies on, for example.

One challenge for "SubForm" objects is that they need to cluster $form_state in separate, isolated entries, so that the business data they place in $form_state doesn't overwrite the stuff placed by the other SubForms - which is what Field API had to do in #942310: Field form cannot be attached more than once to support "fields of several entities in one form" in D7 (see field_form_get_state() / field_form_set_state()). I guess helper getMyOwnFormStateBucket() methods in a base implementation would come a long way make that logic workable.

In order to make e.g. entity forms embeddable couldn't we just do something like

interface EmbeddableFormInterface {
  form();
  validate();
  submit();
}

interface FormInterface extends EmbeddableFormInterface {
  getFormId();
}

class EmbeddableEntityForm implements EmbeddableFormInterface { ... }
class EntityForm extends EmbeddableEntityForm implements FormInterface { ... }

That would also evade the subFormValidate() method naming stuff.

I also had this thought at first, but I don't think we can do that because EmbeddableFormInterface will probably need a method to get the parent form state, and such a method wouldn't make sense to have on FormInterface.

Edit: Hm.. if $form_state is passed along all over the place, maybe that method will not be needed after all.

Edit 2: Nevermind, what @yched said :)

Well, buildForm(), validateForm(), and submitForm() (which are the method names I should have used in #9) all get $form_state passed along. Which makes sense, IMO.

So at least conceptually that should not be a problem. Practically, I agree with @yched that passing only a subset of the actual form state or merging the different states or whatever is not going to be trivial to implement for such a generic system.

Well if we make the interfaces extend each other like I tried to explain in #9 you could do exactly that. Don't know if I've missed anything else major about that, though.

#1938992: Use EmbeddedFormInterface to embed form snippets in forms might try to achieve the same thing.

The reason to not repeat method names are if we want a single class to serve as a standalone and embedded form. If you make the names the same, you can't implement both interfaces.

In order to make e.g. entity forms embeddable couldn't we just do something like

We may be able to make stand-alone forms embeddable, which sounds useful and extremely cool, but I'm trying to think of use cases where someone absolutely does not want a form to be embeddable at all.

A few more reasons to get this issue fixed in Drupal 8:

• Drupal Commerce abuses drupal_get_form() to the point where it introduces some annoying side effects just to embed form snippets.
• I had to mess with #process callbacks in Payment, just to be able to pass on $form_state to plugin's embeddable form builders.

I'm adding the DX tag here, because this issue aims to solve a lot of existing WTFs and custom fixes to the same problem.

What if we add a FormSnippetInterface that contains build, validate, and submit methods and let FormInterface just keep its getFormId() method? This would not break BC.

Alternatively, for consistency with BaseFormIdInterface, we can rename FormInterface to FormIdInterface and still let it implement FormSnippetInterface, but that will break BC.

We have a few good ideas to solve building embedded forms, as well as for storing data in $form_state. However, what do we do with keys like $form_state['rebuild'] and $form_state['redirect']? I imagine that form snippets need to be able to tell the form to rebuild, but not redirect it?

Because it doesn't have to be an API change per se? Also, some API changes still make it in. This obviously would be one that fixes core WTFs.

Having this would remove most of the Inline Entity Form code, a module with 15k installs and a very obvious use case (creating entities as they are being referenced). +1 to whatever, whenever.

My biggest problem right now is that IEF needs to simulate the form processing and call validate and submit methods manually and recursively.

More importantly, one of the functions that you are calling recursively is field_attach_form(), which #2095195: Remove deprecated field_attach_form_*() wants to remove. So you'd have to duplicate even more code. Might actually be a blocker to remove those.

Yes, if field_attach_form dies I'm screwed.

Aren't field form elements attached to entity forms automatically now?

Only if you call it automatically that ContentEntityFormController calls field_attach_form() for you.

And that is actually my point. If ContentEntityFormController is the only way to get that automatism, then we leave modules like Inline Entity Form out in the cold. See field_test_entity_nested_form() for a core example, it's right now impossible to port that to use an EntityFormController.

f_a_form() only deals with configurable fields, it has no reason to stay the moment we start using widgets on base fields too - I mean, IEF does need a fuction/method to be able to do its job, but just acting on configurablw fields is meaningless.

There's one minor detail but major part in the proposal which I don't agree with:

Introduce a new interface similar to FormInterface, but without getFormID().
...
Will there ever be a case where a form is to stand alone or be used as a sub form?

By design, the answer is "Yes." — Here's why:

The one and only point of an EmbeddableFormInterface would be to distill an embedded form from its parent form. It is defined separately, and just happens to be contained elsewhere. The challenge is to figure out (1) whether an embedded form exists, (2) to whom or what it belongs, and (3) dispatch processing of the sub-form in a controlled way.

→ The art of Addressing - or - [reverse-]mapping a data object to a provider. That's the whole point of $form_id.

Identifying a subform within a larger form would theoretically also be possible by turning the top-level $form array into an object, but that is not possible for D8, since it has wide-ranging consequences that would impact the entire rest of the system; specifically, it would break NestedArray and drupal_render().

The aspect of being embeddable does not mean that the form cannot be used on its own. A very typical use-case for using an embeddable form in a standalone way are configurable plugins — it would be pointless to e.g. build the full text format form, just to be able to extract the embedded form of a certain filter plugin.

The scope of the challenge of embedding a form into another is limited exclusively to FormBuilder. Here are the main tasks that need to be addressed:

1. Make it possible to use an element of #type 'form' in a form.
2. When encountering a subform while building a form, dispatch the element to the subform, but retain the #parents.
3. When encountering a subform while validating a form, dispatch validation to the subform, but ensure the subform gets a $form_state as if it was a standalone form.
4. When encountering a subform while submitting a form, dispatch submission to the subform, but ensure the subform gets a $form_state as if it was a standalone form.
5. Lastly, unit test the sh...ebang out of this.

Attached patch is a first prototype and proof of concept, based on Test Driven Development. The unit test proves that the processing of the parent form and the embedded forms does meet each form's individual expectations.

Needless to say, I had to hack my way through FormBuilder to make this possible. Please do not interpret the concrete changes in this patch as an officially proposed implementation — it's an eye-opener only.

The most limiting factor was that $form_state['build_info'] is not "scoped" (sub-keyed) to a particular $form_id. Problems already start in FormBuilder::getFormId(), which injects the FormInterface instance into $form_state → You cannot call FormBuilder::getFormId() for another $form_id using the same $form_state.

And of course, there are a whole range of architectural questions that need to be addressed; each form gets input and values that are scoped to the form, that is out of question. However, what about the remaining $form_state? How much is or should be shared between the parent form and the child forms? Can anything be shared in the first place, or will that cause unforeseen name-clashes of keys in $form_state? Just think of $form_state['entity']... → In this PoC, I opted to selectively share $form_state['storage'] only.

FWIW, this PoC has the additional potential to eliminate and replace the utterly wonky "extract form values" code of field widgets in the Entity Field API. → From a technical perspective, a field form widget is a subform that gets built, validated, and submitted on its own.

Let's discuss. :)

No time for a fully thought out comment here, but see my comment in #2095195-43: Remove deprecated field_attach_form_*(), about why EntityFormController is currently very much a full fledged, standalone form, and cannot be used for a nested subform.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -965,7 +974,19 @@ public function redirectForm($form_state) {
   +      $element = &$elements[$key];
   

   argh I always hate it. so please add comment.

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/MultiFormTest.php
   @@ -0,0 +1,94 @@
   +  public static function getInfo() {
   ...
   +  protected function setUp() {
   ...
   +  protected function render($form) {
   ...
   +  protected function assertRaw($text) {
   
   +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Form/MultiChild1Form.php
   @@ -0,0 +1,66 @@
   +  protected $formBuilder;
   ...
   +  public function __construct($form_builder) {
   
   +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Form/MultiChild2Form.php
   @@ -0,0 +1,66 @@
   +  protected $formBuilder;
   ...
   +  public function __construct($form_builder) {
   
   +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Form/MultiForm.php
   @@ -0,0 +1,87 @@
   +  protected $formBuilder;
   ...
   +  public function __construct($form_builder) {
   

   doc block missing.

3. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/MultiFormTest.php
   @@ -0,0 +1,94 @@
   +    ¶
   

   white space.

4. +++ b/core/modules/system/tests/modules/form_test/form_test.info.yml
   @@ -4,4 +4,4 @@ description: 'Support module for Form API tests.'
   -hidden: true
   +#hidden: true
   

   revert please.

SubformStateInterface landed in core long ago and solved most of the technical problems associated with this. Closing.

Eclipse

This is about embedded forms, not about form state.

I realize that, but given that Plugins would have been the primary beneficiary of this sort of code and that the SubformStateInterface seems to have totally solved its needs, I'm unconvinced that having a separate classification of form type for embedding is worthwhile since the SubformState is the part of that equation that needed solving.

Disagree?

Yes, some APIs have been built without this, but it's a PITA:
* \Drupal\Core\Condition\ConditionPluginBase::buildConfigurationForm
* \Drupal\Core\Condition\ConditionPluginBase::validateConfigurationForm
* \Drupal\Core\Condition\ConditionPluginBase::submitConfigurationForm

* Also \Drupal\media\MediaTypeForm::getSourceSubFormState juggles around naked form arrays.

So let's be nice to the remaining kittens and still do this.

Currently working on something like this in IEF.
Patch #6 by tim.plunkett is exactly what i thought of too.