Document that PluginFormInterface should use #process to solve nesting issues

Before attempting any patching, I would like to hear from tim.plunkett on this.

In which of those examples is $form_state missing?

And how is this different than any other form?

#parents and #array_parents aren't populated until much later.

In which of those examples is $form_state missing?

Once I get the answer to this, we can move forward.

What kind of plugin is that supposed to be? That is not enough code to understand the problem.

Please show me an example of D7 code that used hook_field_instance_settings_form with either #limit_validation_errors or #states, and I will try to rewrite it for D8.

As I see an issue status ping-pong and I want to make sure everyone has a pleasant experience contributing to Drupal, please find me on IRC if necessary and explain the problem to me and I will help. It's often easier than doing it in issue.

So this is not a regression but something missing from the form API in D7 already and exacerbated with the more widespread and canonicalized nature of partial forms -- there are properties like #limit_validation_errors and #states because those need #parents from the root

Working on this.

Here we are. This is only currently needed by blocks, but will needed by image styles after #2251223: ConfigurableImageEffectInterface should use ConfigurablePluginInterface and PluginFormInterface.

Anything not using PluginFormInterface that needs this support needs to switch to it (Views, fields, etc.)

Fixed test

Rerolled for the recently added plugins.

@tim.plunkett and I just discussed this in Austin and I was asked to post my views here.

I don't think this solution is a good one:

1. This approach creates an even closer dependency on Form API to PluginFormInterface.
2. The current solution to solving this problem is making buildConfigurationForm() return an element with a #process callback. By the time the callback is invoked, #parents is already set by Form API and the problem this issue tries to solve is no longer a problem. It takes a few more lines of code, but it re-uses an existing API rather than extending another one.
3. From the point of view of the calling code, passing on the parents to buildConfigurationForm() is not always possible, or at the very least limits the possibilities when altering the form. Passing on an empty array works when the plugin form is top-level. When it is not, the actual parents must be passed on. When these are hardcoded, they must either be changed when altering the form and (re-)nesting the form elements, or the parent form must use #process as well to pass on the parents as they are computed by Form API at build time.

In short, the only two solid solutions require us to use #process. If we have to use that anyway, let's go with the solution that requires no other changes and works just as well, which means this issue is won't fix.

Instead, I suggest we update the PluginFormInterface documentation with the suggestion developers use #process to solve their nesting problems.

We agreed on updating the PluginFormInterface documentation. I'll update a patch shortly.

Since this has become just a docs update, I don't think the tags apply anymore.

What callback do you mean?

For a code example of how to use #process and #parents, see \Drupal\payment\Plugin\Payment\MethodConfiguration\Basic::buildConfigurationForm() and \Drupal\payment\Plugin\Payment\MethodConfiguration\Basic::submitConfigurationForm()

Yes, documenting this solution would indeed be helpful. However, I think it would be better to include this in the documentation for buildConfigurationForm(), not for the interface, since that is where plugin authors look when adding a config form to their plugin – the documentation of the methods they are implementing, not the documentation for every single interface they are implementing.

(Also, updating the summary to the new proposed resolution.)

Re-roll.

I was asked to review this.

A few wording suggestions/questions:

a) contained by other forms ==> contained in other forms

b) "In order to know where the plugin form is located in the parent form..."

I didn't understand this on the first read. I think this should say something like:

When a plugin form is being built, it generally needs to know where it is located in the parent form (#parents and #array_parents properties). In order for this to happen, ...

Thoughts?

That looks good to me! At least, the docs are clear. Marking RTBC so others involved in the issue have a chance to respond before committing.

That is not really a nesting issue, but a naming issue. You should be able to resolve that by prefixing the button's #name value with your module's machine name and by writing a piece of code that generates names for your button and keeps track of the names that are already in use by the form.

Are these issues with #name relevant to the discussion on this issue?

If so, and if we need to add documentation to the topic of this issue, please mark the issue "needs work" and explain what docs need to be added.

Otherwise, please open a new issue or discuss elsewhere. Thanks!

I think it's worth documenting the inconsistent behavior for buttons' #name properties, but that is not related to this issue. Both are in a way about nesting, but the #name issue isn't about nesting render arrays in render arrays, which is what this issue is about.

@Sweetchuck: The #name issue is about Form API and has nothing to do with PluginFormInterface in specific. It is far more generic. Please open a new issue for that.

This issue is opened because the #states and #limit_validation_errors is unusable. There is no kind of documentation which would help this problem.

I agree that Form API has limitations that get in developers' ways. Trust me, I've battled with this before. However, using a #process callback is the only way to solve this right now.

The recommendation to add a new custom #process callback is very weird. That means: "You have to alter your own form elements."

It's not ideal, but it's also not weird. It's how Form API works right now. It's also not about altering your own elements, but about adding to them, depending on how you look at it.

Again, I agree the way Form API does this right now sucks, but we're not going to be able to fix this before 8.0.0, so the best we can do is teach people how to work around this problem.

Form API is large, complex, and somewhat unwieldy.

If you would like me to help you getting #states and #limit_validation_errors working using #process, just ping me on IRC and/or take a look at \Drupal\payment\Plugin\Payment\MethodSelector\AdvancedPaymentMethodSelectorBase::buildSelector().

@Sweetchuck please open a new issue. If there was a backwards compatible way to enable this, then I'd be fine with adding that to a minor 8.x release, but better to have accurate documentation since there's no viable solution for that.

A number of plugins provide form elements to a parent form. These can't use #states or #limit_validation_errors because those need the whole #parents array and the child the form misses the beginning of it.

This is the original report. #states and #limit_validation_errors can be used perfectly fine with Form API in its current state (and even in Drupal 7), as I have explained in several comments on this issue and in several contributed modules.

There is a good reason the required #parents and #array_parents properties do not exist during the initial form build phase. It's far from ideal, but the fix is going to be rather complex and we can't be sure it will be able to get into Drupal 8 at this point, which is right now documenting how to solve your problem is better than spending weeks or months on fixing it with the chance of the fix being rejected.

If the solution we proposed here does not solve your problem, please explain how it doesn't. You keep saying it doesn't solve your problem, but it looks like neither @catch, @jhodgdon, nor me understand how it doesn't. If you simply don't like it, I'm sorry, but that doesn't mean this solution isn't a good one right now.

So I also really dislike #process, but as stated already it's the only way to handle this case - when you're building a plugin form element you don't have access to the whole form. It's a chicken and egg situation and #process gets around it.

Committed/pushed to 8.0.x, thanks!