Allow plugins to provide multiple forms

1. +++ b/core/lib/Drupal/Core/Plugin/PluginFormManagerInterface.php
   @@ -0,0 +1,27 @@
   +/**
   + * Provides form discovery capabilities for block plugins.
   + */
   

   That doesn't sound right.

2. +++ b/core/lib/Drupal/Core/Plugin/PluginFormManager.php
   @@ -0,0 +1,67 @@
   +      // Use the default form class if no form is specified for this operation.
   +      if (isset($definition['form']['default'])) {
   +        $operation = 'default';
   +      }
   +      else {
   +        throw new InvalidPluginDefinitionException($plugin->getPluginId(), sprintf('The "%s" plugin did not specify a "%s" form class', $plugin->getPluginId(), $operation));
   +      }
   

   Having a default form seems weird. If you expect one form to show up and see another... very weird. Would this have any implications with accidentally bypassing permissions on a form? Especially if someone where to proxy the operation from a url param and apply permissions at the route level.

3. +++ b/core/modules/block/src/BlockForm.php
   @@ -402,4 +425,17 @@ public function getUniqueMachineName(BlockInterface $block) {
   +   * Retrieves the plugin form for a given block and operation.
   ...
   +  protected function getPluginForm(BlockPluginInterface $block) {
   +    return $this->pluginFormManager->getFormObject($block, $this->operation);
   

   I wonder for usability/flexibility if it makes sense to take operation as an argument rather then relying on the state of the object?

4. +++ b/core/modules/block/tests/modules/block_test/src/Form/SecondaryBlockForm.php
   @@ -0,0 +1,47 @@
   + * @todo.
   
   +++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestMultipleFormsBlock.php
   @@ -0,0 +1,27 @@
   + * @todo.
   

   heh

5. +++ b/core/tests/Drupal/KernelTests/Core/Block/MultipleBlockFormTest.php
   @@ -0,0 +1,37 @@
   +  /**
   +   * @var array
   +   */
   +  public static $modules = ['system', 'block', 'block_test'];
   

   inheritdoc?

Generic note, the term "operation" I guess makes sense in the terminology in forms but seems weird in the plugin interfaces. We just see "operation" in the method without much explanation as to how that connects to anything. I don't know if it makes sense to change it or not because its in Core not component but its an observation I wanted to put down as I was confused without explanation from Tim.

Generic note, the term "operation" I guess makes sense in the terminology in forms but seems weird in the plugin interfaces.

Yes I think this not the best term. It seems like "context" but we probably don't want to reuse that term here. What about "form_context"?

Having a default form seems weird. If you expect one form to show up and see another... very weird.

I think the intention here is that some plugins might want a different form to show up in different "form_contexts"(if we change the term). But most plugins will not have a special form for a specific form_context.

For example in the related issue #2753941: [Experimental] Create Outside In module MVP to provide block configuration in Off-Canvas tray and expand site edit mode
Certain blocks might want to use a different form in the "offcanvas" form_context. We have to have a default form context because this a new concept that existing plugins won't know about. We can't expect all plugins to know about all form contexts(a new one might be used by contrib). So we have to have "default" form context that we use if the plugin doesn't have a specific one form context being asked for.

+++ b/core/lib/Drupal/Core/Plugin/PluginFormManagerInterface.php
@@ -15,13 +15,15 @@
-   *   The name of the operation to use, e.g., 'default'.
+   *   The name of the operation to use, e.g., 'add' or 'edit'.

Nice, that already helps.

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -81,13 +90,16 @@ class BlockForm extends EntityForm {
   +  public function __construct(EntityManagerInterface $entity_manager, ExecutableManagerInterface $manager, ContextRepositoryInterface $context_repository, LanguageManagerInterface $language, ThemeHandlerInterface $theme_handler, \Drupal\Core\Plugin\PluginFormManagerInterface $plugin_form_manager) {
   

   Can we do a proper use statement and fix the typehint?

2. +++ b/core/modules/block/src/BlockForm.php
   @@ -354,20 +389,8 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   -
   -    // Save the settings of the plugin.
   -    $entity->save();
   -
   -    drupal_set_message($this->t('The block configuration has been saved.'));
   -    $form_state->setRedirect(
   -      'block.admin_display_theme',
   -      array(
   -        'theme' => $form_state->getValue('theme'),
   -      ),
   -      array('query' => array('block-placement' => Html::getClass($this->entity->id())))
   -    );
   

   This and the corresponding add in the diff all seems like an unrelated change, but it's a good one and probably not worth blocking on.

In general, I definitely don't hate this. That's actually probably high praise at this point in my life since mostly I hate things or I don't. :-D Not sure how $this->operation was getting set previous. Does the block entity just default that to default now? Can I change it? or do I need a different wrapper?

Eclipse

So I ran into a problem when I was trying to use this in #2753941: [Experimental] Create Outside In module MVP to provide block configuration in Off-Canvas tray and expand site edit mode
I may have been missing something but here is what I found:

I was trying to make a OffCanvas form class for the SystemBrandingBlock plugin.

My intention was to show the regular plugin settings plus the site name and slogan text fields. The idea being that if someone clicks on the site name and it opens the form in the Offcanvas tray they very well might want edit the site name. If you don't like this approach, the same problem arises if you just want to show a modified version of the plugin form.

The problem I found is because the way \Drupal\Core\Plugin\PluginFormManager and \Drupal\Core\DependencyInjection\ClassResolver work the new form object would have no knowledge of the actual plugin instance it was trying to make a form for. So it can't for instance use the plugin configuration for default form values like SystemBrandingBlock itself does

$form['block_branding']['use_site_logo'] = array(
      '#type' => 'checkbox',
      '#title' => $this->t('Site logo'),
      '#description' => $site_logo_description,
      '#default_value' => $this->configuration['use_site_logo'],
    );

So I created \Drupal\Core\Plugin\PluginAwareInterface

And updated \Drupal\Core\Plugin\PluginFormManager to use in the same way it is using \Drupal\Core\Form\OperationAwareFormInterface

I am not sure if I should have made it PluginAwareFormInterface but I could the interface being used outside the form context.

Thoughts? I am missing an existing way to solve this problem? Is there a better way?

However I see no documentation on how to use this capability. Perhaps PluginFormManagerInterface::getFormObject would be a good place to talk about how to change the annotation?

I disagree with chx a bit. This is not just a nice patch, its like super super super nice! On the longrun it will enable a lot of usecases and make writing plugins less annoying.

1. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -239,9 +239,20 @@ public function useCaches($use_caches = FALSE) {
   +    // Only arrays can be operated on.
   ...
   +      return;
   +    }
   

   I don't see an additional test case in DefaultPluginManager for that. This cannot really be it :)

2. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -239,9 +239,20 @@ public function useCaches($use_caches = FALSE) {
   +    // If no default form is defined and this plugin implements
   +    // \Drupal\Core\Plugin\PluginFormInterface, use that for the default form.
   +    if (!isset($definition['form']['default']) && isset($definition['class']) && is_subclass_of($definition['class'], PluginFormInterface::class)) {
   +      $definition['form']['default'] = $definition['class'];
   +    }
   

   <3 how this enables plugin forms to be separate from the plugins itself. This is just wonderful. To be clear though, on order to make that useful, we need to adapt a lot of the existing plugin codes, which rely plugin instances itself to implement the PluginFormInterface

3. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   new file mode 100644
   index 0000000..f5eb7f2
   
   index 0000000..f5eb7f2
   --- /dev/null
   
   --- /dev/null
   +++ b/core/lib/Drupal/Core/Plugin/PluginAwareInterface.php
   
   +++ b/core/lib/Drupal/Core/Plugin/PluginAwareInterface.php
   +++ b/core/lib/Drupal/Core/Plugin/PluginAwareInterface.php
   @@ -0,0 +1,20 @@
   
   @@ -0,0 +1,20 @@
   +namespace Drupal\Core\Plugin;
   +
   +use Drupal\Component\Plugin\PluginInspectionInterface;
   +
   

   This seems to be generic and could live in the component as well ...

4. +++ b/core/lib/Drupal/Core/Plugin/PluginAwareInterface.php
   @@ -0,0 +1,20 @@
   +   * @param \Drupal\Component\Plugin\PluginInspectionInterface $plugin
   +   *   The plugin.
   +   */
   +  public function setPlugin(PluginInspectionInterface $plugin);
   

   Does this really have to be PluginInspectionInterface ... what about just allowing any kind of object.

5. +++ b/core/lib/Drupal/Core/Plugin/PluginFormManagerInterface.php
   @@ -0,0 +1,29 @@
   +/**
   + * Provides an interface for retrieving form objects for plugins.
   + */
   +interface PluginFormManagerInterface {
   

   Can we tell a bit more about the architecture in the documentation:

   a) This allows to decouple forms from plugin classes
   b) This allows to define multiple forms per plugin.

6. +++ b/core/lib/Drupal/Core/Plugin/PluginFormManagerInterface.php
   @@ -0,0 +1,29 @@
   +   * @param \Drupal\Component\Plugin\PluginInspectionInterface $plugin
   +   *   The plugin the form is for.
   

   It would be nice to document why we need the PluginInspectionInterface here ... I guess its in order to be able to pull out the definition from the plugin?

7. +++ b/core/modules/block/tests/modules/block_test/src/Form/SecondaryBlockForm.php
   @@ -0,0 +1,61 @@
   +/**
   + * Provides a form that is used as a secondary form for a block.
   + */
   

   ... Can we explain what a secondary form does?

8. +++ b/core/modules/block/tests/modules/block_test/src/Form/SecondaryBlockForm.php
   @@ -0,0 +1,61 @@
   +class SecondaryBlockForm implements PluginAwareInterface, PluginFormInterface, OperationAwareFormInterface {
   

   From the definition it looks more like a EmptyBlockForm class.

9. +++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestMultipleFormsBlock.php
   @@ -0,0 +1,27 @@
   + * @Block(
   + *   id = "test_multiple_forms_block",
   + *   form = {
   + *     "secondary" = "\Drupal\block_test\Form\SecondaryBlockForm"
   + *   },
   + *   admin_label = @Translation("Multiple forms test block")
   + * )
   + */
   +class TestMultipleFormsBlock extends BlockBase {
   

   <3

10. +++ b/core/tests/Drupal/KernelTests/Core/Block/MultipleBlockFormTest.php
    @@ -0,0 +1,39 @@
    +
    +    $form_object1 = \Drupal::service('plugin_form.manager')->getFormObject($block, 'default');
    +    $form_object2 = \Drupal::service('plugin_form.manager')->getFormObject($block, 'secondary');
    +
    +    // Assert that the block itself is used for the default form.
    +    $this->assertSame($block, $form_object1);
    +
    +    $expected_secondary = new SecondaryBlockForm();
    +    $expected_secondary->setOperation('secondary');
    +    $expected_secondary->setPlugin($block);
    +    $this->assertEquals($expected_secondary, $form_object2);
    

    For a little bit better readability we could check with assertInstanceOf for the different used classes.

11. +++ b/core/tests/Drupal/Tests/Core/Plugin/PluginFormManagerTest.php
    @@ -0,0 +1,155 @@
    +   * @covers ::getFormObject
    +   */
    +  public function testGetFormObjectDefaultFallback() {
    +    $this->classResolver->getInstanceFromDefinition(Argument::cetera())->shouldNotBeCalled();
    +
    +    $plugin = $this->prophesize(PluginInspectionInterface::class)->willImplement(PluginFormInterface::class);
    +    $plugin->getPluginDefinition()->willReturn([
    +      'form' => [
    +        'fallback' => get_class($plugin->reveal()),
    +      ],
    +    ]);
    +
    +    $form_object = $this->manager->getFormObject($plugin->reveal(), 'missing', 'fallback');
    +    $this->assertSame($plugin->reveal(), $form_object);
    +  }
    

    This could/should maybe be part of the DefaultPluginManager ... on the other hand, why is this default form related code not be part of just PluginFormManager ?

The patch is looking good, love the idea.

Man I don't like being a bummer but we're just tossing interfaces all over the place now. I feel like we're stretching past the original goal of the issue and that makes me nervous. The interdiff in #18 could easily be its own patch with its own CR.

The interdiff in #18 could easily be its own patch with its own CR.

It maybe should be it's own issue but if it was I would think the new issue should block this 1.

There really don't seem to be any point in allowing plugins to have multiple forms if those forms don't have any knowledge of the plugin instance itself. You can't really make a useful for in that case.

+++ b/core/lib/Drupal/Core/Plugin/PluginFormManager.php
@@ -0,0 +1,71 @@
+    // Ensure the resulting object is a plugin form.
+    if (!$form_object instanceof PluginFormInterface) {
+      throw new InvalidPluginDefinitionException($plugin->getPluginId(), sprintf('The "%s" plugin did not specify a valid "%s" form class, must implement \Drupal\Core\Plugin\PluginFormInterface', $plugin->getPluginId(), $operation));
+    }

I was even thinking about changing this to enforce that $form_object be a instance of PluginAwareInterFace. Though maybe there is use case I am not thinking of.

Great job on getting this started!
I actually reinvented this API for Commerce Payment just two days ago.
Stand-by for opinions.

Generic note, the term "operation" I guess makes sense in the terminology in forms but seems weird in the plugin interfaces.

I struggled with this too, and ended up going with $form_id since it felt more generic, but it doesn't feel significantly clearer (if at all).

There really don't seem to be any point in allowing plugins to have multiple forms if those forms don't have any knowledge of the plugin instance itself. You can't really make a useful for in that case.

+1000. Plugin forms should be plugin aware by default. I'd go as far as injecting $plugin before the dependencies, but I guess setter injection does the job too.

+ *   form = {
+ *     "secondary" = "\Drupal\block_test\Form\EmptyBlockForm"
+ *   },

We should think about the right convention for namespacing plugin forms. Feels odd to dump them in "Form" when they aren't real forms, they don't implement the real FormInterface.
So how about "PluginForm"? With an optional (or required?) grouping by plugin class?
That way modules can do Drupal\mymodule\PluginForm\EmptyBlockForm or even Drupal\mymodule\PluginForm\MyBlock\EmptyBlockForm (if there are multiple plugins that need to declare the same form).

+class PluginFormManager implements PluginFormManagerInterface {

Isn't this technically the PluginFormFactory? We usually use Manager to mean "I had no clue what to name this class", but a more clear alternative is available here. And since getFormObject() is returning new instances, it might make more sense for it to be createForm() or createInstance().

+    $definition = $plugin->getPluginDefinition();

Would be great to have some kind of PluginWithFormsInterface that gives us getFormClass() and hasFormClass(), that can then be used by the PluginFormManager.

+    // If no default form is defined and this plugin implements
+    // \Drupal\Core\Plugin\PluginFormInterface, use that for the default form.
+    if (!isset($definition['form']['default']) && isset($definition['class']) && is_subclass_of($definition['class'], PluginFormInterface::class)) {
+      $definition['form']['default'] = $definition['class'];
+    }

Why not 'configuration' instead of 'default'? It is the configuration form, which has a lot more meaning than 'default'.

+  /**
+   * {@inheritdoc}
+   */
+  public function getFormObject($plugin, $operation, $fallback_operation = NULL) {
+    if (!$plugin instanceof PluginInspectionInterface) {
+      throw new \InvalidArgumentException(sprintf('The plugin must implement "%s"', PluginInspectionInterface::class));
+    }

This immediately looked weird. Why are we not type hinting the PluginInspectionInterface? Then I saw above that it was dawehner's suggestion, and discussed it with him on IRC. I think that it makes total sense to type hint $plugin with PluginInspectionInterface. There is no reason not to have a minimal interface that says "this object is a plugin", and currently that's PluginInspectionInterface, it's extended by every single plugin type interface we have. It introduces no additional coupling. I think dawehner agrees now, but I won't put words in his mouth. So, let's reconsider (both here and for setPlugin() in the PluginAwareInterface).

Missing/improper docblocks:

+  /**
+   * PluginFormManager constructor.
+   *
+   * @param \Drupal\Core\DependencyInjection\ClassResolverInterface $class_resolver
+   */
+  public function __construct(ClassResolverInterface $class_resolver) {

+  /**
+   * @var string
+   */
+  protected $operation;
+
+  /**
+   * @var object
+   */
+  protected $plugin;

Now on a more subjective note. It feels messy to try and support both the old configuration-form-on-the-plugin and the new plugin-form-outside-the-plugin approach in this API. Just look at the PluginFormInterface naming, buildConfigurationForm() makes no sense on plugin forms, since most plugin forms are not configuration forms. You usually have one central "configuration" form, the rest is its own thing.

Ideally I would do something like this:
1) Create a PluginConfigurationFormInterface as a copy of the current PluginFormInterface, deprecate the current PluginFormInterface methods in their favor. Switching all of core could be a single followup.
2) Add the more generic methods (buildForm / validateForm / submitForm) to PluginFormInterface
3) Repurpose PluginFormManager to only support the external forms (and fallback between them).
4) Convert the block configuration form to an external form in order to perform the fallback needed by your UI.

This gives us a clear distinction between the configuration form VS plugin forms, for cases where it matters.

EDIT: D'oh, actually that won't work cause the external plugin forms would still need to implement the old deprecated methods. So we might be stuck with the ugly-ugly-naming unless we invent a whole different interface for external forms.
Still, #3 and #4 make sense.

Awesome. I've been working on something similar.

1. +++ b/core/lib/Drupal/Core/Plugin/PluginFormFactory.php
   @@ -0,0 +1,67 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function createInstance(PluginInspectionInterface $plugin, $operation, $fallback_operation = NULL) {
   

   Can we add a PluginFormFactory::hasForm() method to determine if a plugin has a form without catching the exception?

2. +++ b/core/lib/Drupal/Core/Plugin/PluginFormFactory.php
   @@ -0,0 +1,67 @@
   +    // If the form specified is the plugin itself, use it directly.
   +    if (get_class($plugin) === $definition['form'][$operation]) {
   +      $form_object = $plugin;
   +    }
   

   Does this need an ltrim($definition['form'][$operation], '\\')?

The code looks spot on. Since I don't have any real criticism, here are my own meandering thoughts.

+++ b/core/lib/Drupal/Core/Plugin/PluginFormBase.php
@@ -8,6 +8,9 @@
+ *
+ * Classes extending this can be in any namespace, but are commonly placed in
+ * the 'PluginForm' namespace, such as \Drupal\module_name\PluginForm\ClassName.

Interesting choice. I've been using PluginDir\Form.

1. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -239,9 +239,20 @@ public function useCaches($use_caches = FALSE) {
   +    // If no default form is defined and this plugin implements
   +    // \Drupal\Core\Plugin\PluginFormInterface, use that for the default form.
   +    if (!isset($definition['form']['configuration']) && isset($definition['class']) && is_subclass_of($definition['class'], PluginFormInterface::class)) {
   +      $definition['form']['configuration'] = $definition['class'];
   +    }
   

   Question: Does 'configure' make more sense than 'configuration', when considered along with 'add' and 'edit'?

2. +++ b/core/lib/Drupal/Core/Plugin/PluginFormFactory.php
   @@ -0,0 +1,91 @@
   +  /**
   +   * PluginFormFactory constructor.
   +   *
   

   I would love it if this is how we roll now.

Thanks for taking this on! There are a few things I believe are important to discuss to make sure this approach is conceptually sound:

• If we introduce a new API for plugin forms, consider leveraging #2653106: Introduce SubformInterface. It can be made compatible with PluginFormInterface by wrapping it with a decorator in PluginFormFactory::createInstance().
• This decorator approach can also be used to address the method naming issue @bojanz raised in #27.
• Consistency with entity forms, as entity types using the same structure in their definitions.

Patch review:

1. +++ b/core/lib/Drupal/Component/Plugin/PluginAwareInterface.php
   @@ -0,0 +1,18 @@
   + * Interface for objects that are aware of a plugin.
   

   This needs rewriting to the third person singular.

   I'm struggling to describe awareness myself, but I think we need to try and come up with something more descriptive than the current comment.

2. +++ b/core/lib/Drupal/Component/Plugin/PluginAwareInterface.php
   @@ -0,0 +1,18 @@
   +   * @param \Drupal\Component\Plugin\PluginInspectionInterface $plugin
   

   Plugin instances do not necessarily need to implement PluginInspectionInterface. Could we support that here, or is there no way around this?

3. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -239,9 +239,20 @@ public function useCaches($use_caches = FALSE) {
   +    // Only arrays can be operated on.
   +    if (!is_array($definition)) {
   +      return;
   +    }
   

   This needs support for \Drupal\Component\Plugin\Definition\PluginDefinitionInterface.

4. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -239,9 +239,20 @@ public function useCaches($use_caches = FALSE) {
   +
   +    // If no default form is defined and this plugin implements
   +    // \Drupal\Core\Plugin\PluginFormInterface, use that for the default form.
   +    if (!isset($definition['form']['configuration']) && isset($definition['class']) && is_subclass_of($definition['class'], PluginFormInterface::class)) {
   +      $definition['form']['configuration'] = $definition['class'];
   +    }
   

   This needs support for \Drupal\Component\Plugin\Definition\PluginDefinitionInterface.

5. +++ b/core/lib/Drupal/Core/Plugin/PluginFormBase.php
   @@ -0,0 +1,38 @@
   +  protected $plugin;
   

   $pluginInstance

6. +++ b/core/lib/Drupal/Core/Plugin/PluginFormFactory.php
   @@ -0,0 +1,91 @@
   +   * PluginFormFactory constructor.
   

   I'm not sure we should use class names in one-line constructor summaries, as they are invalid for child classes and cannot be automatically refactored.

7. +++ b/core/lib/Drupal/Core/Plugin/PluginFormFactory.php
   @@ -0,0 +1,91 @@
   +    // If the form specified is the plugin itself, use it directly.
   +    if (ltrim(get_class($plugin), '\\') === ltrim($form_class, '\\')) {
   +      $form_object = $plugin;
   +    }
   +    else {
   +      $form_object = $this->classResolver->getInstanceFromDefinition($form_class);
   +    }
   

   Weird, but I like it!

8. +++ b/core/lib/Drupal/Core/Plugin/PluginFormFactory.php
   @@ -0,0 +1,91 @@
   +      throw new InvalidPluginDefinitionException($plugin->getPluginId(), sprintf('The "%s" plugin did not specify a valid "%s" form class, must implement \Drupal\Core\Plugin\PluginFormInterface', $plugin->getPluginId(), $operation));
   

   Thanks for thinking about error handling! Seeing as plugin IDs are ambiguous if the plugin type is unknown, could we make this a bit more specific by including the plugin class name as well? It's not perfect, but it definitely helps according the handful community members who've complained to me about other such errors in the Plugin System, as it's a better indicator of where they need to search for the cause of the problem.

9. +++ b/core/lib/Drupal/Core/Plugin/PluginFormFactoryInterface.php
   @@ -0,0 +1,48 @@
   + * This allows a plugin to define multiple forms, in addition to the plugin
   + * itself providing a form. All forms, decoupled or self-contained, must
   + * implement \Drupal\Core\Plugin\PluginFormInterface. Decoupled forms can
   + * implement \Drupal\Component\Plugin\PluginAwareInterface in order to gain
   + * access to the plugin.
   

   I like this bit about PluginAwareInterface! Can we document this on PluginFormInterface as well?

10. +++ b/core/lib/Drupal/Core/Plugin/PluginFormFactoryInterface.php
    @@ -0,0 +1,48 @@
    +  public function createInstance(PluginInspectionInterface $plugin, $operation, $fallback_operation = NULL);
    

    What about renaming this to ::getform() for consistency with ::hasFormClass() and EntityFormBuilder::getForm()?

11. +++ b/core/lib/Drupal/Core/Plugin/PluginFormFactoryInterface.php
    @@ -0,0 +1,48 @@
    +  public function hasFormClass(PluginInspectionInterface $plugin, $operation);
    

    What about ::hasForm()? It's shorter and calling code does not have to know about how the check is performed internally.

12. +++ b/core/lib/Drupal/Core/Plugin/PluginFormFactoryInterface.php
    --- a/core/modules/block/src/BlockForm.php
    +++ b/core/modules/block/src/BlockForm.php
    

    Thanks for including an example conversion. Just a heads-up about a future re-roll: this conflicts with #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms), which was RTBC'd just this morning.

13. +++ b/core/modules/block/src/BlockForm.php
    @@ -69,6 +71,13 @@ class BlockForm extends EntityForm {
    +  protected $pluginFormManager;
    

    $pluginFormFactory

Thanks again for working on this useful feature :)

Yay for the namespace change.

I still think hasFormClass and getFormClass methods belong on the plugin itself. After all, it's the plugin definition they are analyzing.
Feels weird to ask a factory if it has a form class. And making it $factory->hasForm() is even more confusing, makes me think it caches the created instances.

Questions:

+  /**
+   * Sets the plugin for this object.
+   *
+   * @param \Drupal\Component\Plugin\PluginInspectionInterface $plugin
+   *   The plugin.
+   */
+  public function setPlugin(PluginInspectionInterface $plugin);

Should setPlugin return $this, like setters usually do?

+ *   form = {
+ *     "secondary" = "\Drupal\block_test\PluginForm\EmptyBlockForm"
+ *   },

Shouldn't the definition key be "forms" instead of "form"? We tend to use plurals in these cases (multiple values).
Guessing it's "form" cause of the entity annotation, but there it's nested under "handlers".

I still think hasFormClass and getFormClass methods belong on the plugin itself. After all, it's the plugin definition they are analyzing.

"it's the plugin definition they are analyzing" is exactly the reason why it doesn't belong in the plugin. Personally, I'd be fine with moving this to DefaultPluginManager, or DefaultFormPluginManager, or something. There is an issue here with the ability to override how a form is derived per-plugin-type.

Should setPlugin return $this, like setters usually do?

Fluent interfaces should be designed specifically for the purpose. Don't pollute the stack.

Awesome! +1 for RTBC.

I agree with bojanz

Read through the code and actually all seems pretty straight-forward to me. The one part I got pretty confused by was:

+/**
+ * Provides a block with multiple forms.
+ *
+ * @Block(
+ *   id = "test_multiple_forms_block",
+ *   forms = {
+ *     "secondary" = "\Drupal\block_test\PluginForm\EmptyBlockForm"
+ *   },
+ *   admin_label = @Translation("Multiple forms test block")
+ * )
+ */
+class TestMultipleFormsBlock extends BlockBase {

...it's not clear to me how this actually defines multiple forms. Tim explained that there's a "magic" config = ... form set in the background. Fair enough. Fortunately, the new change record has a much better example.

Seems like this patch has lots of support from plugin system maintainers, as well as people with lots of experience implementing entities, and the test coverage is quite extensive, so... :)

Committed and pushed to 8.2.x (after removing an unused use statement - thanks https://github.com/alexpott/d8githooks!). Thanks!

#37: Thanks! Those are good improvements :)

I still think hasFormClass and getFormClass methods belong on the plugin itself. After all, it's the plugin definition they are analyzing.

I've never liked this approach (also elsewhere, such as PluginInspectionInterface::getId()), because PluginInspectionInterface::getDefinition() gets us all the information we need without adding more methods to plugin classes that are often too big already. This also means plugin classes that provide multiple forms now require a dependency on \Drupal\Core instead of just \Drupal\Component.

What's the difference? I'm not renaming it to setPluginInstance(), I think this is fine.

So far I've understood we call plugins what we discover, which generally means the definition/class, and instances are literal class instances of these plugins.

I raised #2653106: Introduce SubformInterface in #34. Is there a reason that was not taken into account for this issue? What technical reasons are there to use PluginFormInterface rather than a solution that builds on #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms)'s SubformStateInterface?

+++ b/core/lib/Drupal/Core/Plugin/PluginFormFactoryInterface.php
@@ -0,0 +1,35 @@
+  public function createInstance(PluginWithFormsInterface $plugin, $operation, $fallback_operation = NULL);

I wanted to ask this yesterday, but a browser failure threw away my comment and I had forgotten to include it when I re-wrote everything: why the fallback operation? There is no non-test usage of this. What are examples of wanting to fall back to a different form automatically?

We use this ALL OVER. Take it up in a doc standards issue.

It's not a standards issue. It would be nice to learn why you disagreed with the suggestion.

+++ b/core/lib/Drupal/Component/Plugin/PluginAwareInterface.php
@@ -0,0 +1,18 @@
+<?php
+
+namespace Drupal\Component\Plugin;
+
+/**
+ * Provides an interface for objects that depend on a plugin.
+ */
+interface PluginAwareInterface {
+
+  /**
+   * Sets the plugin for this object.
+   *
+   * @param \Drupal\Component\Plugin\PluginInspectionInterface $plugin
+   *   The plugin.
+   */
+  public function setPlugin(PluginInspectionInterface $plugin);
+
+}

Shoot this went in fast! Last I checked this was failing :(. A little disappointed because I wanted to discuss this more because "aware" could mean "give me a plugin" or "let me handle a plugin" and I'm not really comfortable with where it sits still.

That means it's consistent, but not necessary that it's good. Both @neclimdul and I raised this issue now, and that's not because we're new to the concept.

"Aware" is pretty standard speak for the interfaces that allow setter injection (which is all this is).
Not sure we can do better while still doing setter injection.
The only real alternative is using a custom class resolver to pass the $plugin before all other parameters in __construct / create (just like we do for $plugin_id and friends in plugin classes).

Note this patch has some negative consequences at #2796953: [regression] Plugins extending from classes of uninstalled modules lead to fatal error during discovery.

On the other hand, this should be really helpful to allow contrib to implement forms for the core migration plugins...