Problem/Motivation

PluginFormInterface allows plugins to build their own forms. It accepts form elements and the form state.

The interface does not clearly document the form state is the complete form's state and not the state of the plugin form alone. This is because form states were never designed to be for form subsets only.

Pretending form states are for subsets, creates problems like #2532968: Block plugin forms assume they are top-level, where forms create fake subset form states by copying the complete form's state selectively, causing data loss in the process.

Proposed resolution

Introduce SubformStateInterface and implement it in SubformState, which decorates the parent form's state (which itself may also be a subform). Any state behavior that is specific to a certain form level (parent, sub, sub-sub,....), will be overridden, but all changes will also be synchronized with the ancestor/complete form state internally. This allows parent forms to create a limited form state scope for subforms, parent forms and subforms to work indepently of one another (decreasing coupling and increasing reusability), and limits the actions parent forms must taken to one additional line of code per subform (the creation of the subform state).

Remaining tasks

None.

User interface changes

N/A

API changes

API additions only.

Data model changes

None.

RC Target because:

The pattern of creating new FormState for plugins is fundamentally flawed, and not a pattern we want to trickle down to contrib.
This would also be almost impossible to fix from contrib, all core plugins referenced by other objects are affected.

CommentFileSizeAuthor
#126 interdiff.txt1.11 KBtim.plunkett
#126 2537732-plugin-126.patch103.17 KBtim.plunkett
#123 2537732-123--subform_states.patch103.07 KBdrunken monkey
#123 2537732-123--subform_states--interdiff.txt5.65 KBdrunken monkey
#118 2537732-plugin-118.patch101.75 KBtim.plunkett
#114 drupal_2537732_114.patch101.56 KBXano
#114 interdiff.txt790 bytesXano
#110 drupal_2537732_110.patch101.92 KBXano
#110 interdiff.txt992 bytesXano
#109 drupal_2537732_108.patch101.8 KBXano
#109 interdiff.txt3.06 KBXano
#105 drupal_2537732_105.patch101.46 KBXano
#105 interdiff.txt1.11 KBXano
#104 drupal_2537732_104.patch100.61 KBXano
#102 drupal_2537732_102.patch100.67 KBXano
#102 interdiff.txt580 bytesXano
#100 drupal_2537732_100.patch100.63 KBXano
#100 interdiff.txt3.71 KBXano
#97 drupal_2537732_97.patch103.43 KBXano
#97 interdiff.txt10.97 KBXano
#95 drupal_2537732_95.patch100.47 KBXano
#95 interdiff.txt1.98 KBXano
#91 drupal_2537732_91.patch100.43 KBXano
#91 interdiff.txt63.61 KBXano
#87 drupal_2537732_22_8.2.x.patch92.46 KBXano
#87 drupal_2537732_22_8.1.x.patch92.46 KBXano
#87 drupal_2537732_22_8.0.x.patch92.46 KBXano
#87 interdiff_8.0.x.patch25.99 KBXano
#80 drupal_2537732_80.patch90.04 KBXano
#80 interdiff.txt5.12 KBXano
#79 drupal_2537732_79.patch94.7 KBXano
#79 interdiff.txt24.62 KBXano
#75 pluginforminterface-2537732-75.patch86 KBgnuget
#75 2537732-interdiff-74-75.txt810 bytesgnuget
#74 interdiff.txt4.27 KBclaudiu.cristea
#74 2537732-74.patch85.81 KBclaudiu.cristea
#73 error_input.png26.27 KBgnuget
#69 drupal_2537732_69.patch82.09 KBXano
#69 interdiff.txt27.86 KBXano
#58 drupal_2537732_58.patch80.73 KBXano
#58 interdiff.txt44.7 KBXano
#56 drupal_2537732_56.patch47.87 KBXano
#49 2537732-form_state-49.patch47.92 KBtim.plunkett
#47 2537732-form_state-47.patch47.92 KBtim.plunkett
#39 drupal_2537732_39.patch47.56 KBXano
#39 interdiff.txt781 bytesXano
#31 drupal_2537732_31.patch47.26 KBXano
#31 interdiff.txt14.19 KBXano
#29 drupal_2537732_29.patch46.99 KBXano
#27 drupal_2537732_27.patch46.88 KBXano
#27 interdiff.txt5.91 KBXano
#25 drupal_2537732_25.patch47.47 KBXano
#25 interdiff.txt14.95 KBXano
#23 just-for-review-do-not-test.patch17.13 KBtim.plunkett
#23 interdiff.txt10.34 KBtim.plunkett
#23 2537732-form_state-23.patch46.57 KBtim.plunkett
#21 interdiff.txt25.04 KBtim.plunkett
#21 2537732-form_state-21.patch46.28 KBtim.plunkett
#19 interdiff.txt7.6 KBtim.plunkett
#19 2537732-form_state-19.patch26.82 KBtim.plunkett
#15 drupal_2537732_15.patch25.87 KBXano
#15 interdiff.txt8.75 KBXano
#12 drupal_2537732_12.patch24.9 KBXano
#5 2537732-form_state_stack-5.patch10.05 KBtim.plunkett
#2 drupal_2537732_2.patch1.83 KBXano
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Xano’s picture

Status: Active » Needs review
FileSize
1.83 KB
EclipseGc’s picture

Status: Needs review » Closed (works as designed)

Through out core, we provide a new FormState() with extracted values specific to the plugin's form in order to solve this basic need. No plugin should have to be responsible for anticipating its use in any given situation. That responsibility resides with the code leveraging a plugin type with configuration forms. This is why core's implementations for these use cases are ALWAYS accompanied by the new FormState() methodology. In short, I'd say that core does this correctly. This behavior conforms to basic Plugin philosophy through out all of core. I've experienced no problems using plugins of this sort outside of core's use cases. We were very careful to ensure that would be true. Just use the proscribed method and you should be fine. It's much nicer than having to edit every single validate and submit handler manually.

Eclipse

Xano’s picture

Status: Closed (works as designed) » Active
Related issues: +#2338837: Fix SubFormState

Moving back to active as we had a good discussion on IRC in which we uncovered some of the pros and cons of both approaches:

Creating a new 'sub' form state:

  • con: All form state state is lost, except for the submitted values. This prevents plugins from storing any temporary data between the build, validate, and submit phases. Plugin's plugin selectors are an example of code that relies on this functionality heavily. This is bad DX, because the form state is type-hinted FormStateInterface, but most of that interface won't be usable by plugin forms.
  • pro: no extra code needed in plugin forms.

Using NestedArray:

  • con: reduced DX due to the necessity to include an additional line of code in every implementation of PluginFormInterface::validateConfigurationForm() and PluginFormInterface::submitConfigurationForm().
  • pro: full form state functionality retained.

A third suggested solution is to make form state values stackable. This is possible, but we need to make absolutely sure submitted form values are the only nested data contained by form states.

@amateescu's good memory dug up #2338837: Fix SubFormState in which this was discussed before.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
10.05 KB

Needs docs updates still, but how about this?

jhodgdon’s picture

Um, is that the right patch in #5? Doesn't seem to be docs or related to the previous patch.

tim.plunkett’s picture

Since #4 I think we're currently discussing changing the way core works, not documenting how it works (since no one is happy with the current state).
My patch was in response to "A third suggested solution is to make form state values stackable" and was indeed written from scratch, not building on the previous patch.

jhodgdon’s picture

Ah. Well we need a new issue summary/title then.

tim.plunkett’s picture

Title: PluginFormInterface must document $form_state contains the complete form's state » PluginFormInterface must have access to the complete $form_state
Issue summary: View changes
Issue tags: -Needs issue summary update

Did my best to retitle and update the IS.

EclipseGc’s picture

I am very very ++ to #5, fwiw. It keeps the implementation details in the calling code, doesn't require us to embed code into every validate/submit function to ever reside on a plugin anywhere, and gives FormStateInterface a native approach to dealing with processing sub-form-values. ++

Eclipse

Xano’s picture

I think the code in #5 is very nifty and solves the issue at hand.

Perhaps we can do even better by indeed working with form states from the parent forms, but creating a completely new child FormStateInterface instance that decorates the parent form state:

  • Introduce FormStateInterface::getFormStateForElement(array $element) which produces the decorator form state, specific to the given element. The method name is self-descriptive. The default implementation decorates the original form state and routes all method calls to the decorated object, but the contained values are a reference to the nested values array in the decorated form state.
  • This would require only a single method call from the parent form, as opposed to two calls in the patch from #5. The parent form state remains unchanged, and as such will also not have to be reverted to any previous state.
  • Both form state objects can live at the same time, without interfering with each other.

I will post a patch with this approach tomorrow. It seems we will have some difficulties with the static methods on FormStateInterface, though. Seeing as we can have multiple FormSate instances per request, and those static methods are meant to preserve global state, I'm not sure they should live on that class.

Xano’s picture

This implements the idea coined in #11. The patch does not contain tests or implementations of this functionality yet.

Status: Needs review » Needs work

The last submitted patch, 12: drupal_2537732_12.patch, failed testing.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Form/FormStateManager.php
@@ -0,0 +1,39 @@
+class FormStateManager implements FormStateManagerInterface {

Meh. I don't see this change as in scope or necessary.

The rest is interesting, can we see it in action? Maybe start with image effects.

Xano’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
8.75 KB
25.87 KB

Meh. I don't see this change as in scope or necessary.

If we support processing/validating multiple form states at the same time, it doesn't make much sense to access FormState directly. I did simplify the code a bit by removing the service and interface, but keeping a dedicated FormStateManager class to keep track of global form validation errors.

The rest is interesting, can we see it in action? Maybe start with image effects.

Aye, aye!

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -1084,29 +1084,23 @@ public function getRedirect() {
    -    static::getFormStateManager()->setAnyErrors($errors);
    +    FormStateManager::setAnyErrors($errors);
    

    Hmm I guess this makes some sense. I still think it's out of scope.

  2. +++ b/core/modules/image/src/Form/ImageEffectFormBase.php
    @@ -31,7 +31,7 @@
    -   * @var \Drupal\image\ImageEffectInterface
    +   * @var \Drupal\image\ConfigurableImageEffectInterface
    

    This isn't actually true, it can be either. Use | to separate the typehints please!

  3. +++ b/core/modules/image/src/Form/ImageEffectFormBase.php
    @@ -79,7 +79,8 @@ public function buildForm(array $form, FormStateInterface $form_state, ImageStyl
    -    $form['data'] = $this->imageEffect->buildConfigurationForm(array(), $form_state);
    +    $sub_form_state = new SubFormState($form_state, ['data'], ['data']);
    +    $form['data'] = $this->imageEffect->buildConfigurationForm(array(), $sub_form_state);
    

    Hmm, I certainly get the validate/submit change, but is there any functional change by doing this? build should absolutely not be touching values anyway...

  4. +++ b/core/modules/image/src/Form/ImageEffectFormBase.php
    @@ -108,10 +109,7 @@ public function buildForm(array $form, FormStateInterface $form_state, ImageStyl
    -    $effect_data = (new FormState())->setValues($form_state->getValue('data'));
    -    $this->imageEffect->validateConfigurationForm($form, $effect_data);
    -    // Update the original form values.
    -    $form_state->setValue('data', $effect_data->getValues());
    +    $this->imageEffect->validateConfigurationForm($form, $form_state->getFormStateForElement($form['data']));
    

    Beautiful!

tim.plunkett’s picture

I should say, while I was very pleased with my patch in #5, I think this approach is also worth exploring. We'll see how it shakes out, and hopefully @effulgentsia or @neclimdul could chime in once both are full-fledged.

Status: Needs review » Needs work

The last submitted patch, 15: drupal_2537732_15.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
26.82 KB
7.6 KB

Went looking to see if this issue helped #2538816: $form_state->getValues() is empty when '#tree = FALSE' in $form (it does not, not right now anyway), and updated some more.

Not sure of my changes to SubFormState::getValues, but in #15 it is infinitely recursive.

Xano’s picture

Thanks, this looks really good!

+++ b/core/modules/image/src/Form/ImageEffectFormBase.php
@@ -79,8 +79,7 @@ public function buildForm(array $form, FormStateInterface $form_state, ImageStyl
-    $sub_form_state = new SubFormState($form_state, ['data'], ['data']);
-    $form['data'] = $this->imageEffect->buildConfigurationForm(array(), $sub_form_state);
+    $form['data'] = $this->imageEffect->buildConfigurationForm(array(), $form_state);

I'm not really happy with this change. While currently there is no practical need to create a subform state during build phases (because this currently only affects submitted form values), this may change in the future and we don't want to have to update all form builders then.
The solution from #15 is quite ugly though, and ideally we'd design a more developer-friendly way of creating a subform state during the build phase, when #parents and #array_parents are not known yet.

tim.plunkett’s picture

We *do* need something better. I just removed that for now...

I wrote some unit tests for SubFormState. I don't know why we need #array_parents, is it just for future-proofing?
Also SubFormState::getValues() is the only place we need to do magic.
In order to reuse all of the other value-related methods, I moved them to a trait.

However, in doing so I found a problem.
Given this code:

$form['foo'] = [
  '#type' => 'textfield',
  '#value' => 'bar',
];

$sub_form_state = $form_state->getFormStateForElement($form['foo']);
$values = $sub_form_state->getValues();
$value = $sub_form_state->getValue(NULL);

$values will be 'bar', a string.
The getValue() call will then try to pass it to NestedArray::getValue, which will throw a 'invalid type operand' error.

So, how do we prevent getFormStateForElement() from being used on a form element that doesn't contain any other form elements?!

Status: Needs review » Needs work

The last submitted patch, 21: 2537732-form_state-21.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
46.57 KB
10.34 KB
17.13 KB

That last patch was missing a bunch of changes. Whoops.

I think this check in createForFormElement() should be adequate. But is it the right thing to do?

Also attached a patch without FormStateDecoratorBase (since it is a pure decorator) and all test changes, to make the patch somewhat easier to review.

Xano’s picture

I don't know why we need #array_parents, is it just for future-proofing?

Yes. This issue started out as a form state values problem, but if we want to make sure this is absolutely future-proof during the D8 cycle, we need to abstract this out a little. It's probably cleanest to make SubFormState::__construct() protected, but then we no longer have an alternative to the element-based factory method during build phases at this moment.

Also SubFormState::getValues() is the only place we need to do magic.

For now. We may be adding additional logic to form states in minor releases. It's not likely anyone is going to override FormBuilder to change the default FormState implementation, so FormStateInterface changes in ~8.1.0 releases don't seem out of the question.

  1. +++ b/core/lib/Drupal/Core/Form/SubFormState.php
    @@ -55,9 +56,17 @@ public function __construct(FormStateInterface $parent_form_state, array $array_
    +   * @throws \InvalidArgumentException
    +   *   Thrown when the form element does not wrap other form elements, or if it
    +   *   does not have the necessary Form API properties assigned.
    

    I know why this was done, but plugin forms are technically allowed to return anything that is a valid render array, including empty arrays or arrays that are just one element. Parent forms should not have to care about that. The only code that would ever try to get specific values from the subform state is the subform itself, which also knows which elements it has provided in the first place.

  2. +++ b/core/tests/Drupal/Tests/Core/Form/SubFormStateTest.php
    @@ -142,32 +167,27 @@ public function providerTestGetValue() {
           'Dodger',
    

    :D

Xano’s picture

This patch creates a subform state by subform reference, and lazily retrieves the #parents and #array_parents so it's a little more flexible. In also enables plugin forms to access nested subform state information by implementing #process themselves if that need arises in the future. It's still not a perfect solution to the absence of #parents and #array_parents during the build phase, though.

Status: Needs review » Needs work

The last submitted patch, 25: drupal_2537732_25.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
5.91 KB
46.88 KB
tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Form/SubFormState.php
    @@ -18,63 +18,76 @@ class SubFormState extends FormStateDecoratorBase {
    +  protected function getSubFormParents() {
    ...
    +  protected function getSubFormStateParents() {
    

    I think the difference between these is really confusing. Yes, the difference between #parents and #array_parents is *also* confusing, but can't we just name these after the property?

  2. +++ b/core/modules/block/src/BlockForm.php
    @@ -125,7 +125,9 @@ public function form(array $form, FormStateInterface $form_state) {
    -    $form['settings'] = $entity->getPlugin()->buildConfigurationForm(array(), $form_state);
    ...
    +    $form['settings'] = $entity->getPlugin()->buildConfigurationForm($form['settings'], $subform_state);
    
    @@ -285,7 +287,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    -    $this->entity->getPlugin()->validateConfigurationForm($form, $form_state->getFormStateForElement($form['settings']));
    +    $this->entity->getPlugin()->validateConfigurationForm($form['settings'], $form_state->getFormStateForSubform($form['settings']));
    
    @@ -309,7 +311,7 @@ protected function validateVisibility(array $form, FormStateInterface $form_stat
    -      $condition->validateConfigurationForm($form, $form_state->getFormStateForElement($form['visibility'][$condition_id]));
    +      $condition->validateConfigurationForm($form['visibility'][$condition_id], $form_state->getFormStateForSubform($form['visibility'][$condition_id]));
    
    @@ -322,13 +324,13 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -    $entity->getPlugin()->submitConfigurationForm($form, $form_state->getFormStateForElement($form['settings']));
    +    $entity->getPlugin()->submitConfigurationForm($form['settings'], $form_state->getFormStateForSubform($form['settings']));
    ...
    -      $condition->submitConfigurationForm($form, $form_state->getFormStateForElement($form['visibility'][$condition_id]));
    +      $condition->submitConfigurationForm($form['visibility'][$condition_id], $form_state->getFormStateForSubform($form['visibility'][$condition_id]));
    
    +++ b/core/modules/image/src/Form/ImageEffectFormBase.php
    @@ -79,7 +79,9 @@ public function buildForm(array $form, FormStateInterface $form_state, ImageStyl
    -    $form['data'] = $this->imageEffect->buildConfigurationForm(array(), $form_state);
    ...
    +    $form['data'] = $this->imageEffect->buildConfigurationForm($form['data'], $subform_state);
    
    @@ -108,7 +110,7 @@ public function buildForm(array $form, FormStateInterface $form_state, ImageStyl
    -    $this->imageEffect->validateConfigurationForm($form, $form_state->getFormStateForElement($form['data']));
    +    $this->imageEffect->validateConfigurationForm($form, $form_state->getFormStateForSubform($form['data']));
    
    @@ -119,7 +121,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -    $this->imageEffect->submitConfigurationForm($form, $form_state->getFormStateForElement($form['data']));
    +    $this->imageEffect->submitConfigurationForm($form, $form_state->getFormStateForSubform($form['data']));
    

    In the last two examples, you still pass $form through, but not in the first couple. I'm not sure which I think is more correct, but we should be consistent.

  3. +++ b/core/tests/Drupal/Tests/Core/Form/SubFormStateTest.php
    @@ -67,42 +67,14 @@ public function testCreateForFormElement() {
    -   * @expectedException \InvalidArgumentException
    -   * @expectedExceptionMessage $element must contain the #array_parents key
    -   */
    -  public function testCreateForFormElementMissingArrayParents() {
    

    What about testing the new InvalidArgumentException in getSubFormProperty()?

  4. +++ b/core/tests/Drupal/Tests/Core/Form/SubFormStateTest.php
    @@ -126,23 +98,23 @@ public function providerTestGetValues() {
    -    $data = [];
    -    $data['exist'] = [
    -      ['foo'],
    -      $this->initialValues['foo'],
    -    ];
    -    $data['nested'] = [
    -      ['dog', 'name'],
    -      'Dodger',
    +    return [
    +      [['foo'], $this->initialValues['foo']],
    +      [['dog', 'name'], 'Dodger'],
         ];
    -    return $data;
    

    Can you please revert this change (and the rest of the dataProviders)? When you key the return of a provider with something sensible, it becomes infinitely easier to find than 0, 1, 2 etc.

Xano’s picture

FileSize
46.99 KB

Re-roll. I will address the feedback from #28 in another patch.

Status: Needs review » Needs work

The last submitted patch, 29: drupal_2537732_29.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
14.19 KB
47.26 KB

I think the difference between these is really confusing. Yes, the difference between #parents and #array_parents is *also* confusing, but can't we just name these after the property?

I updated the method names to reflect the names of the array keys.

In the last two examples, you still pass $form through, but not in the first couple. I'm not sure which I think is more correct, but we should be consistent.

I updated the patch so PluginFormInterface always receives the subform instead of the parent form.

What about testing the new InvalidArgumentException in getSubFormProperty()?

You already did that in ::testGetValuesBroken(), but I updated the @covers annotations to reflect this.

Can you please revert this change (and the rest of the dataProviders)? When you key the return of a provider with something sensible, it becomes infinitely easier to find than 0, 1, 2 etc.

I'd never thought of that before, but it makes sense. I reverted these changes.

In #2532968: Block plugin forms assume they are top-level I added a web test to assert that plugin form implementations support embedding. Since this issue has practically replaced that other one, do we want to add similar tests in this patch?

Status: Needs review » Needs work

The last submitted patch, 31: drupal_2537732_31.patch, failed testing.

EclipseGc’s picture

We've spent a bit of time on this at this point. I'm unconvinced by this approach. It will absolutely work (so in that sense I'm convinced) but I'm still not sure I see benefit to it over tim's push/pop method. I just wanted to bring this up and see if we have a consensus on why this solution is better (if we indeed have such a thing). Thanks for humoring me.

Eclipse

Xano’s picture

Tim's method works, but is somewhat unclear semantically, is more difficult to extend in the future, and requires calling code to 'reset' the form state after the child form has been processed. The advantage of sub-form states is that they can co-exist with parent form states, theoretically allowing code to work on both objects at the same time, but more importantly, this does not require calling code to 'reset' the form state afterwards. This results in better DX and lower chance at bugs.

Xano’s picture

#2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager) forgot to include a plugin configuration schema, which causes the test failures here. #2547581: Missing configuration schemas for condition plugins fixes that, after which the patch here should be green.

borisson_’s picture

The way this looks in the implementation looks cleaner, I think this is a good improvement. Do we want to open followups to convert other usages or should this be done in this issue?

  1. +++ b/core/lib/Drupal/Core/Form/SubFormState.php
    @@ -0,0 +1,117 @@
    +   * Creates a new instance for a subform.
    

    Creates a new instance of SubFormState or extending class for a subform. would be a better comment, I think.

  2. +++ b/core/modules/image/src/Form/ImageEffectFormBase.php
    @@ -121,10 +120,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -    $effect_data = (new FormState())->setValues($form_state->getValue('data'));
    -    $this->imageEffect->submitConfigurationForm($form, $effect_data);
    -    // Update the original form values.
    -    $form_state->setValue('data', $effect_data->getValues());
    +    $this->imageEffect->submitConfigurationForm($form['data'], $form_state->getSubFormState($form['data']));
    

    +1

Status: Needs work » Needs review

Xano queued 31: drupal_2537732_31.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 31: drupal_2537732_31.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
781 bytes
47.56 KB
Xano’s picture

Another thing we can do now is scope FormStateInterface::set() and ::get(), so there can be no clashes between parent and embedded forms.

bojanz’s picture

From an Inline Entity Form perspective, I personally never had a problem with using NestedArray and requiring developers to know they're in a subform.
This patch still won't hide that fact from people who want to use #ajax, or have multiple subforms open (risking collisions in the parent form state).

On the other hand, I can see how this patch reduces fragility in core, so here's a tentative +1. Unlike the one in #5 the current one is something IEF could leverage.

Thoughts:
- There's currently no way to get the parent form state values from a subform state. This is sometimes needed (consulting the parent language, title, etc).
We could fix this by simply introducing a method that returns the decorated / parent form state.
- FormStateDecoratorBase most like won't ever be used outside of SubFormState, so maybe there's an argument for merging them?
Would also make it easier to document the rationale in the SubFormState docblock (which we definitely need to do).

Xano’s picture

This patch still won't hide that fact from people who want to use #ajax, or have multiple subforms open (risking collisions in the parent form state).

How exactly?

There's currently no way to get the parent form state values from a subform state. This is sometimes needed (consulting the parent language, title, etc).
We could fix this by simply introducing a method that returns the decorated / parent form state.

While I understand that this is, for whatever reason, a practical need, it violates the API of PluginFormInterface. It sounds that this can be solved by making the parent form inject such dependencies into the plugins, by using a custom form interface specifically for this relationship between parent and embedded form that documents that the embedded form can only be embedded in a specific parent form, or by using a custom form state decorator that provides methods to access this information.

FormStateDecoratorBase most like won't ever be used outside of SubFormState, so maybe there's an argument for merging them?

The reason I split them up was to keep the potential for future decorators (Maybe SubFormState won't suffice in all cases, as outlined above), and to keep SubFormState (which contains the logic this is all about) more readable, not 'obscured' by the simple decorations.

Xano’s picture

Xano queued 39: drupal_2537732_39.patch for re-testing.

tim.plunkett’s picture

Category: Task » Bug report
Priority: Normal » Major

I think the "other methods on $form_state are completely ignored" thing is a convincingly bad problem.

Xano’s picture

Another thing we can do now is scope FormStateInterface::set() and ::get(), so there can be no clashes between parent and embedded forms.

Should we do this, or not?

tim.plunkett’s picture

Straight reroll.

Xano queued 47: 2537732-form_state-47.patch for re-testing.

tim.plunkett’s picture

Issue summary: View changes
Issue tags: +rc target triage
FileSize
47.92 KB

Reroll.

Anonymous’s picture

I have closed a ticket that I have created yesterday #2617466: BlockForm is ignoring block plugin form errors. in favour of this one but I just want to stress out that at this time the form validation is broken and should be fixed asap. Even though this looks like a good approach I am not sure how long will it take for this story to get out there so if you feel it will take too long, please reopen my ticket again since that is an actual and currently existing bug that should get fixed right away.

tim.plunkett’s picture

Reviewing my patch would help.

dsnopek’s picture

Looking at #4:

Creating a new 'sub' form state:

con: All form state state is lost, except for the submitted values. This prevents plugins from storing any temporary data between the build, validate, and submit phases. Plugin's plugin selectors are an example of code that relies on this functionality heavily. This is bad DX, because the form state is type-hinted FormStateInterface, but most of that interface won't be usable by plugin forms.

I'm not sure I understand this... Couldn't the SubFormState just defer directly to the parent FormStateInterface for all other operations besides values? For example:

class SubFormState implements FormStateInterface {

  // ...

  public function setTemporaryValue($name, $value) {
    $this->parent->setTemporaryValue($name, $value);
  }
}

Or am I missing something?

Xano’s picture

@dsnopek: That's a very old comment, from before the current approach even existed. I don't remember what I meant by that.

dsnopek’s picture

@Xano: Oh, ok, cool! I was talking with tim.plunkett about this on IRC and he said that using a SubFormState like in #2338837: Fix SubFormState was proposed here, and best I could tell #4 was the comment where that approach was discussed (since it adds the reference to that issue). :-)

Now that 8.0.0 is out, and this change has to be made without affecting backwards compatibility, I personally think adding a SubFormState class sounds like a workable solution! If there are any current objections to that approach, I'd love to discuss them.

EDIT: Actually, looking at your patch from #31, it's pursuing a SubFormState approach! That's that's probably a good place to reset to if we can't follow Tim's approach for backcompat reasons.

davidwbarratt’s picture

Status: Needs review » Needs work

I applied the patch from #49 and I still was not able to save block config in a Plugin field.

Xano’s picture

Re-roll. I'm also working on some test coverage.

davidwbarratt’s picture

Status: Needs work » Needs review

Kicking off the test bot. :)

Xano’s picture

Now with full test coverage for FormStateDecoratorBase.

I think we'll need to improve FormStateValuesTrait a bit. It stores the submitted form values in an internal property, which is only useful for FormState, because we'll need to override ::getValues() in SubFormState in order to retrieve the parent form state's values and extract the subset. If we keep FormStateValuesTrait::getValues() abstract, we move the current implementation to FormState, and we use the trait in FormStateDecoratorBase

Xano’s picture

Title: PluginFormInterface must have access to the complete $form_state » PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms)
Xano’s picture

#2647464: Condition plugin configuration forms depend on their parent forms explains why condition plugin forms in block forms should be broken by this patch, but there are no test failures for this.

Xano’s picture

We must document that arbitrary callbacks such as #process, #submit, and #element_validate, which are called by form API, will still receive the complete form state.

Xano’s picture

Status: Needs review » Needs work

This patch breaks BC, because of the introduction of FormStateInterface::getSubformState(). That must be removed. We can use SubformState::createForSubform() instead.

chx’s picture

Status: Needs work » Needs review

Adding a method to this interface... Do you expect people will write their own FormState objects not extending FormState?

tim.plunkett’s picture

Search API has done it, to work around this exact issue. That's where we got the idea for this implementation.

mallezie’s picture

Xano’s picture

@chx: Yes, I do. As @tim.plunkett said, Search API has done it, and I did it this weekend to work around a Views-ism. Regardless of what we expect, that method addition was a BC break and it wasn't for the best possible reason.

@tim.plunkett: I didn't actually know about Search API's SubFormState until a week or two ago :)

claudiu.cristea’s picture

Nice!

Some notes, most of them nits.

  1. +++ b/core/lib/Drupal/Core/Form/FormStateDecoratorBase.php
    @@ -0,0 +1,697 @@
    +  public function setFormState(array $form_state_additions) {
    +    $this->decoratedFormState->setFormState($form_state_additions);
    +
    +    return $this;
    +  }
    

    Nit: I think for all the methods with 2 lines of code from this class we can omit the empty line before return. It would be more compact but still readable. See the coding standards example here: https://www.drupal.org/coding-standards#functdecl

  2. +++ b/core/lib/Drupal/Core/Form/FormStateValuesTrait.php
    @@ -0,0 +1,104 @@
    +   * flat array or an array whose structure parallels the $form array. See
    +   * \Drupal\Core\Render\Element\FormElement for more information.)
    

    Let's add (or move this) a @see statement because that will provide a clickable link in the IDE and also on api.drupal.org.

  3. +++ b/core/lib/Drupal/Core/Form/FormStateValuesTrait.php
    @@ -0,0 +1,104 @@
    +  protected $values = array();
    

    Short array syntax? I know it was moved around but it's also "new code" :)

  4. +++ b/core/lib/Drupal/Core/Form/SubFormState.php
    @@ -0,0 +1,117 @@
    +    }
    +
    +
    +    return $values;
    
    +++ b/core/lib/Drupal/Core/Plugin/PluginFormInterface.php
    @@ -32,7 +32,9 @@
        *   The form structure.
    

    Remove one empty line.

  5. +++ b/core/modules/block/src/BlockForm.php
    @@ -125,7 +125,9 @@ public function form(array $form, FormStateInterface $form_state) {
    +    $form['settings'] = [];
    +    $subform_state = $form_state->getSubFormState($form['settings']);
    +    $form['settings'] = $entity->getPlugin()->buildConfigurationForm($form['settings'], $subform_state);
    
    +++ b/core/modules/image/src/Form/ImageEffectFormBase.php
    @@ -78,7 +78,9 @@ public function buildForm(array $form, FormStateInterface $form_state, ImageStyl
    +    $form['data'] = [];
    +    $subform_state = $form_state->getSubFormState($form['data']);
    +    $form['data'] = $this->imageEffect->buildConfigurationForm($form['data'], $subform_state);
    

    I see a pattern here. Why not adding a factory static method on FormState to do all these 3 lines in one? I guess this will be the same on each form with plugin subforms:

    $form[$key] = FormState::getPluginSubForm($key, $plugin, $form_state);
    

    or something like this?

  6. +++ b/core/modules/block/src/BlockForm.php
    @@ -285,11 +287,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
         // settings form element, so just pass that to the block for validation.
    
    +++ b/core/tests/Drupal/Tests/Core/Form/FormStateDecoratorBaseTest.php
    @@ -0,0 +1,1226 @@
    \ No newline at end of file
    

    Newline.

  7. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateDecoratorBaseTest.php
    @@ -0,0 +1,1226 @@
    +  public function testClearErrors() {
    ...
    +  public function testGetErrors() {
    

    These tests have no assertions. I think PHP Unit will mark them as unsafe.

  8. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateValuesTraitTest.php
    @@ -0,0 +1,218 @@
    +    $element = array(
    +      '#parents' => array(
    ...
    +    $expected = array(
    +      'foo' => array(
    ...
    +      'bar' => array(
    ...
    +    $data = array();
    +    $data[] = array(
    ...
    +    $data[] = array(
    +      array('bar', 'baz'), 'two',
    ...
    +    $data[] = array(
    +      array('foo', 'bar', 'baz'), NULL,
    ...
    +    $data[] = array(
    ...
    +    $data[] = array(
    ...
    +    $data = array();
    +    $data[] = array(
    +      'foo', 'one', array('bar' => 'wrong', 'foo' => 'one'),
    ...
    +    $data[] = array(
    +      array('bar', 'baz'), 'two', array('bar' => array('baz' => 'two')),
    ...
    +    $data[] = array(
    +      array('foo', 'bar', 'baz'), NULL, array('bar' => 'wrong', 'foo' => array('bar' => array('baz' => NULL))),
    ...
    +      'bar' => array(
    ...
    +    $data = array();
    +    $data[] = array(
    ...
    +    $data[] = array(
    +      array('bar', 'baz'), TRUE,
    ...
    +    $data[] = array(
    +      array('foo', 'bar', 'baz'), FALSE,
    ...
    +    $data[] = array(
    ...
    +    $data[] = array(
    ...
    +    $data[] = array(
    ...
    +      'bar' => array(
    ...
    +    $data = array();
    +    $data[] = array(
    ...
    +    $data[] = array(
    +      array('bar', 'baz'), FALSE,
    ...
    +    $data[] = array(
    +      array('foo', 'bar', 'baz'), TRUE,
    ...
    +    $data[] = array(
    ...
    +    $data[] = array(
    ...
    +    $data[] = array(
    

    Short array syntax [].

  9. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateValuesTraitTest.php
    @@ -0,0 +1,218 @@
    +  public function providerTestGetValue() {
    ...
    +  public function providerTestSetValue() {
    ...
    +  public function providerTestHasValue() {
    ...
    +  public function providerTestIsValueEmpty() {
    

    Missing doc-blocks for data providers.

EDIT: And, yes, the BC break

Xano’s picture

Thanks for the review! Those are good points, and I'll incorporate them in the next patch.

I see a pattern here. Why not adding a factory static method on FormState to do all these 3 lines in one? I guess this will be the same on each form with plugin subforms:

Adding them to FormState, but not FormStateInterface prevents a BC break, but is a little silly, because it makes FormState depend on SubFormState. We should move the factory (if we want to keep any) to SubFormState, and make it depend on FormStateInterface, so our classes are decoupled. Mental note: subforms are not restricted to plugins.

Xano’s picture

FileSize
27.86 KB
82.09 KB

Let's add (or move this) a @see statement because that will provide a clickable link in the IDE and also on api.drupal.org.

Short array syntax? I know it was moved around but it's also "new code" :)

I moved this code back to FormState, because FormStateValuesTrait made too many assumptions about variable storage. These decorators are tricky enough without a misleading and unused $values property.

These tests have no assertions. I think PHP Unit will mark them as unsafe.

I added an assertion to one of the methods, but the other one really only decorates the method call. There is no return value, so we cannot assert anything.

This patch fixes the issues raised by @claudiu.cristea, unless otherwise noted. It also makes sure the code consistently uses subform as a single word. It introduces SubformInterface with a single method to get the complete/global/parent form state to match FormStateInterface::getCompleteForm(). I have not seen any use case for subforms to access the global form state (dependencies should be injected properly instead of potentially passed on through arbitrary form state value arrays, creating dependencies between classes), but the feedback of some people made me think that adding this method would be a compromise that means existing code can switch to SubformState with as few disruptions as possible.

Xano’s picture

Xano’s picture

Issue tags: +Subforms
Xano’s picture

gnuget’s picture

Status: Needs review » Needs work
FileSize
26.27 KB
+++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
@@ -73,6 +73,9 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
+    }

+++ b/core/tests/Drupal/Tests/Core/Form/FormStateValuesTraitTest.php
@@ -0,0 +1,267 @@
+
+  /**
+   * Provides data to self::testHasValue().
+   * ¶
+   * @return array[]
+   *   Items are arrays of two items:
+   *   - The key to check for in the form state (string)
+   *   - Whether the form state has an item with that key (bool).
+   */
+  public function providerHasValue() {

When I applied this patch a message showed up, I think is because this space.

Also, With this patch the inputs aren't colored on red when they have errors.
error input

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
85.81 KB
4.27 KB
gnuget’s picture

The #2649746: Block config form does not show errors from blockValidate's test didn't cover the error class in the input, I fixed that in this new patch.

Status: Needs review » Needs work

The last submitted patch, 75: pluginforminterface-2537732-75.patch, failed testing.

Xano’s picture

Ah, of course. We've got to decorate the error methods as well.

Xano’s picture

I am looking into this, and realized that the current code does not support subform states based on subform states (subforms in subforms), because we rely on subforms' #parents and #array_parents keys, but their values are relative to the ancestor/global form, and not to the parent form.

Xano’s picture

Status: Needs work » Needs review
FileSize
24.62 KB
94.7 KB

This patch improves test coverage, documentation, variable naming, and fixes the bug that the test failure from #75 exposed (thanks, @gnuget!).

Xano’s picture

Fear my copy/paste skillz!

borisson_’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Form/SubformState.php
    @@ -0,0 +1,160 @@
    +  public static function createForSubform(array &$subform, array &$parent_form, FormStateInterface $parent_form_state) {
    +    return new static($subform, $parent_form, $parent_form_state);
    +  }
    

    Why do we need to have this method? This doesn't add any value as far as I can see.

  2. +++ b/core/lib/Drupal/Core/Form/SubformStateInterface.php
    @@ -0,0 +1,27 @@
    +
    +
    

    There should be only one blank line here.

  3. +++ b/core/modules/block/src/Tests/BlockUiTest.php
    @@ -293,4 +293,19 @@ public function testBlockPlacementIndicator() {
    +  /**
    +   * Tests if validation errors are passed from the 'settings' subform to the
    +   * block configuration form.
    +   */
    +  public function testBlockValidateErrors() {
    

    The summary of the docblock should be only one line.

  4. +++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestSettingsValidationBlock.php
    @@ -0,0 +1,43 @@
    +  public function blockValidate($form, FormStateInterface $form_state) {
    

    This method should get a docblock.

  5. +++ b/core/modules/image/src/Tests/ImageEffectsTest.php
    @@ -168,6 +169,27 @@ function testImageEffectsCaching() {
    +   * Tests if validation errors are passed from effect object validation to
    +   * effect form.
    

    The method description could be one line.

  6. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateDecoratorBaseTest.php
    @@ -0,0 +1,1230 @@
    +   * The subject under test.
    

    /s/subject/system

Manjit.Singh’s picture

Is it be the part of 8.2.x branch ?

Manjit.Singh’s picture

Is it be the part if 8.2.x branch ?

tim.plunkett’s picture

Version: 8.0.x-dev » 8.2.x-dev

Well it's not going into 8.0.x or 8.1.x for sure.
No telling if it will be ready for 8.2.x or not.

Manjit.Singh’s picture

Issue tags: +Needs reroll
Xano’s picture

Version: 8.2.x-dev » 8.1.x-dev

As long as this is a bug report, this is allowed for 8.1.x.

Xano’s picture

Why do we need to have this method? This doesn't add any value as far as I can see.

I wanted to keep the constructor protected, so we can easily change it in the future. Doing that required a public factory method. The reason for keeping the constructor protected is that this is a complex issue and while all this seems to work and we haven't found any shortcomings for a while, I'm not entirely sure we won't realize there are still unsolved problems in the future.

/s/subject/system

I changed the entire property name and its documentation, since in another issue people expressed a dislike to it. I am still in favor of SUT for its portability (no need to change it when the name of the class under test changes, forinstance), so if we can get a core committer sign-off on that after all, I would like to change it back.

I set this issue to 8.1.x under the impression that that was the original version, but it wasn't, so I am setting it back to 8.0.x, which also means that the patch does not need a re-roll at this time. I would like a core committer to clarify which branches this patch must target. Because this is a bug report, I assume that we must fix this in 8.2.x and 8.1.x, and maybe even 8.0.x if that is still supported by the time this patch is committed.

The last submitted patch, 87: interdiff_8.0.x.patch, failed testing.

tim.plunkett’s picture

It's an API addition, no chance for 8.0.x
But I'll let a committer tell you that.

bojanz’s picture

Agreed, on march 26th the chances of this coming near 8.0.x are long gone.

Xano’s picture

I converted the tests to use Prophecy (except when testing methods that return values by reference), and split off coverage of completely different API methods. I also fixed an @covers annotation, and added missing implementations to FormStateDecoratorBase.

Status: Needs review » Needs work

The last submitted patch, 91: drupal_2537732_91.patch, failed testing.

The last submitted patch, 91: drupal_2537732_91.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Xano’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
100.47 KB
borisson_’s picture

Status: Needs review » Needs work

The other questions I had about this were answered by @Xano at drupalcamp spain.

  1. +++ b/core/lib/Drupal/Core/Form/FormStateValuesTrait.php
    @@ -0,0 +1,88 @@
    +  /**
    +   * @see \Drupal\Core\Form\FormStateInterface::getValues()
    +   */
    

    I think these need an actual description as well.

  2. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateDecoratorBaseTest.php
    @@ -0,0 +1,1570 @@
    +/**
    + * @file
    + * Contains \Drupal\Tests\Core\Form\FormStateDecoratorBaseTest.
    + */
    

    These should be removed. There's more @file blocks in this patch.

Xano’s picture

Status: Needs work » Needs review
FileSize
10.97 KB
103.43 KB

Thanks @borisson_ for the review, and to @bojanz for pointing out (on IRC) that FormStateInterface::getUserInput() and FormStateInterface::setUserInput() would need to be decorated as well. This patch addresses the given feedback, and adds additional test coverage for SubformState::getValue() and SubformState::getValues() to verify that they return by reference.

Status: Needs review » Needs work

The last submitted patch, 97: drupal_2537732_97.patch, failed testing.

Xano’s picture

I've tried to come up with solutions to decorate FormStateInterface::getUserInput(), but none of them work. The problem is that no input exists by default, but we must return by reference. When getting the input for a subform, we cannot create an entry in the parent's user input to reference, because that would mean the parent's input is no longer empty, because it will then contain empty child arrays for the subform. As far as I can tell, to make this work, we must do one of the following:

  1. SubFormState::getUserInput() returns the global input, which prevents us from getting the subform user input directly.
  2. Remove the reference, so public function &getUserInput() becomes public function &getUserInput(). This should work, because FormStateInterface::setUserInput() exists as well, but breaks BC.
Xano’s picture

Status: Needs work » Needs review
FileSize
3.71 KB
100.63 KB

We concluded we cannot properly decorate the user input methods, so let's do it without that for now.

Status: Needs review » Needs work

The last submitted patch, 100: drupal_2537732_100.patch, failed testing.

Xano’s picture

Status: Needs review » Needs work

The last submitted patch, 102: drupal_2537732_102.patch, failed testing.

Xano’s picture

Xano’s picture

Xano’s picture

Any other concerns about this approach?

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
    @@ -47,7 +48,7 @@ public function isNegated() {
    +    $contexts = ($form_state instanceof SubformStateInterface ? $form_state->getCompleteFormState()->getTemporaryValue('gathered_contexts') : $form_state->getTemporaryValue('gathered_contexts')) ?: [];
    

    Why do we need this?

    +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
    @@ -68,6 +69,9 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
    +    if ($form_state->hasValue('context_mapping')) {
    +      $this->setContextMapping($form_state->getValue('context_mapping'));
    +    }
    

    And this?

    These are the only confusing changes to implementation.
    Perhaps just a comment would help.

  2. +++ b/core/lib/Drupal/Core/Form/SubformState.php
    @@ -0,0 +1,155 @@
    +    return $this->decoratedFormState instanceof SubformStateInterface ? $this->decoratedFormState->getCompleteFormState() : $this->decoratedFormState;
    

    Nice forward thinking. Does a double nested form_state have coverage in this patch?

  3. +++ b/core/lib/Drupal/Core/Plugin/PluginFormInterface.php
    @@ -27,7 +27,9 @@
    +   *   The current state of the form. Calling code should pass on a sub form
    

    We need to decide between "sub-form" or "subform", but "sub form" is definitely wrong.

Otherwise I think it's ready to mark RTBC. Thanks @Xano for sticking with this!

bojanz’s picture

+  /**
+   * Gets the complete form state.
+   *
+   * @return \Drupal\Core\Form\FormStateInterface
+   *
+   * @deprecated Deprecated as of Drupal 8.1.x. Scheduled for removal before
+   *   Drupal 9.0.0. Subforms should not depend on another form's form state,
+   *   and any dependencies should be passed on through the forms' API methods.
+   */
+  public function getCompleteFormState();

It feels odd to add a new method that's immediately deprecated.
We agreed we want to have getCompleteFormState(), we are even using it in the new code (ConditionPluginBase).
So can we just remove the deprecation?

Xano’s picture

Thanks for reviewing this patch again!

+++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
@@ -47,7 +48,7 @@ public function isNegated() {
+    $contexts = ($form_state instanceof SubformStateInterface ? $form_state->getCompleteFormState()->getTemporaryValue('gathered_contexts') : $form_state->getTemporaryValue('gathered_contexts')) ?: [];

Why do we need this?

Some top-level forms communicate with their subforms through the complete/top-level form state storage. This code assures that the storage of the complete form state is accessed rather than the current or parent form state. I expect that this pattern will have to be used in several other places, including contrib. @bojanz has given us some examples in #41.

+++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
@@ -68,6 +69,9 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
+    if ($form_state->hasValue('context_mapping')) {
+      $this->setContextMapping($form_state->getValue('context_mapping'));
+    }

And this?

BlockForm has always been responsible for processing its embedded forms' values. This moves that responsibility to the plugins themselves, but keeps the old implementation for backwards compatibility. I have added a code comment in BlockForm to indicate this should be removed in Drupal 9.

Nice forward thinking. Does a double nested form_state have coverage in this patch?

Yes, see \Drupal\Tests\Core\Form\SubformStateTest::testGetCompleteFormStateWithParentSubform().

We need to decide between "sub-form" or "subform", but "sub form" is definitely wrong.

I standardized on subform earlier on in this issue, and corrected the sub form occurrences you pointed out.

Xano’s picture

It feels odd to add a new method that's immediately deprecated.
We agreed we want to have getCompleteFormState(), we are even using it in the new code (ConditionPluginBase).
So can we just remove the deprecation?

We crossposted, so I wasn't able to implement your feedback in #109. Here it is.

bojanz’s picture

The changes in #109 look good.

I can't really parse the comment added in #110:

+   * @todo Nested forms should not have to be aware of each other. Find a
+   *   solution to make Form API pass on specific implementations of this
+   *   interface instead of \Drupal\Core\Form\FormState for every embedded
+   *   form's callbacks, so those classes' API methods can be used for
+   *   communication between parent forms and subforms.

The point of this method is not for nested forms to be aware of each other, it's for nested forms to be aware of the parent form.
For the same reason why we pass $complete_form in various form callbacks.
Is the point of the comment to say that we should have a way of injecting the complete form state based on a type hint?

Xano’s picture

You're right about subforms not having to be aware of each other. After submitting I realized what we're looking for ideally is (like) dependency injection from parent forms to subforms, so what about something like this?
Subforms should not have to be aware of their parents. Find a solution to make Form API pass on specific implementations of this interface to subforms' callbacks instead of \Drupal\Core\Form\FormState, so parent forms can inject dependencies through those classes instead of through the complete form state.

bojanz’s picture

I still think finding a way to avoid access to the parent form state is not a battle worth fighting, just like we are not trying to remove $complete_form from callbacks. Your @todo will end up as just another never-resolved arbitrary suggestion in our codebase.

But I can live with it. +1 to RTBC if nobody else has anything to add.

Xano’s picture

FileSize
790 bytes
101.56 KB

Since the thought of never-resolved arbitrary suggestions sends shivers down my spine, I agree with @bojanz it's best to avoid those.

jonathanshaw’s picture

Status: Needs review » Reviewed & tested by the community
tim.plunkett’s picture

Version: 8.1.x-dev » 8.2.x-dev

+1 for RTBC.

xjm’s picture

Issue tags: +Needs change record

I mentioned to @tim.plunkett that a CR could help with this issue.

tim.plunkett’s picture

Xano’s picture

Issue tags: -Needs change record

Change record added at https://www.drupal.org/node/2774077.

catch’s picture

Had a decent look through this, in general it looks like a great addition, and this is something that's horrible to deal with usually.

However this is also the first time we're formally introducing the concept of subforms in core, so I think the new classes could do with some more docs to explain what they are etc. Don't love 'subform' but don't have a better idea. The whole concept of 'form within a form' is tricky because it's not a proper HTML form within a form, but it is a Drupal form (in that a Drupal form builder is specifically not 1-1 with HTML forms so that this can happen), so the naming is fine but a bit more high level documentation would be good.

  1. +++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.php
    @@ -47,7 +48,7 @@ public function isNegated() {
    +    $contexts = ($form_state instanceof SubformStateInterface ? $form_state->getCompleteFormState()->getTemporaryValue('gathered_contexts') : $form_state->getTemporaryValue('gathered_contexts')) ?: [];
    

    Don't love nested ternaries - have to scroll a full screen width in dreditor to get to the end of this one.

  2. +++ b/core/lib/Drupal/Core/Form/SubformState.php
    @@ -0,0 +1,155 @@
    + * Stores information about the state of a subform.
    

    This could use a bit more docs. Right now this is the only class in core that references subforms (grep finds mentions in field UI, views, editor modules), so either here or somewhere else appropriate should document the new formal concept we're introducing. Especially given it's not valid HTML to have a form tag within another form tag, so 'subform' is pretty specific to form API.

  3. +++ b/core/lib/Drupal/Core/Form/SubformState.php
    @@ -0,0 +1,155 @@
    +  protected $parent_form;
    

    not $parentForm?

  4. +++ b/core/lib/Drupal/Core/Form/SubformState.php
    @@ -0,0 +1,155 @@
    +  protected $subform;
    

    Not $subForm?

  5. +++ b/core/lib/Drupal/Core/Form/SubformState.php
    @@ -0,0 +1,155 @@
    +    $relative_subform_parents = $this->subform[$property];
    +    // Remove all of the subform's parents that are also the parent form's
    +    // parents, so we are left with the parents relative to the parent form.
    +    foreach ($this->parent_form[$property] as $parent_form_parent) {
    +      if ($parent_form_parent !== $relative_subform_parents[0]) {
    +        // The parent form's parents are the subform's parents as well. If we
    +        // find no match, that means the given subform is not contained by the
    +        // given parent form.
    +        throw new \UnexpectedValueException('The subform is not contained by the given parent form.');
    +      }
    +      array_shift($relative_subform_parents);
    +    }
    

    Had a think about different ways to do this, but they were all worse. So yeah fair enough.

  6. +++ b/core/lib/Drupal/Core/Form/SubformStateInterface.php
    @@ -0,0 +1,17 @@
    + * Stores information about the state of a subform.
    

    Again this is a bit scant.

  7. +++ b/core/modules/block/src/Tests/BlockUiTest.php
    @@ -288,4 +288,18 @@ public function testBlockPlacementIndicator() {
    +    $this->assertTrue(!empty($elements));
    +    $this->assertTrue(!empty($error_class));
    

    Why the !empty?

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +beta deadline

CNW for docs (not for the nitpicks), and would be good to at least discuss the naming briefly (someone in irc suggested 'form fragment') also tagging beta deadline since this is a large API addition.

Xano’s picture

Not $subForm?

would be good to at least discuss the naming briefly (someone in irc suggested 'form fragment') also tagging beta deadline since this is a large API addition.

I'm on mobile, so can't pull historical logs easily, but this has been done. The name sub form was chosen at some point and there were no serious complaints. I'd be okay with changing the capitalization, but not to postpone this for yet another six months on this, considering the integration benefits this issue provides.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
5.65 KB
103.07 KB

This should address all complaints from #120, except this one:

Not $subForm?

Then we'd also have to change the class and interface name, parameter/variable names and all doc text mentions.
Personally, I like "sub form" also better than "subform", but I don't think it's worth changing this now.

I tried my best describing the concept of sub forms in the interface doc comment in an understandable way. Not sure if I succeeded, though – you be the judge of that.

Anyways, would be great to still get this into 8.2.0. Otherwise, we'll just get more and more modules adding their own copy of this class.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @drunken monkey

alexpott’s picture

+++ b/core/modules/block/src/Tests/BlockUiTest.php
@@ -298,8 +298,8 @@ public function testBlockValidateErrors() {
-    $this->assertTrue(!empty($elements));
-    $this->assertTrue(!empty($error_class));
+    $this->assertTrue($elements);
+    $this->assertTrue($error_class);

This is a nit that could be fixed on commit but assertTrue with no message is a really bad pattern because you just get TRUE is TRUE - which is, well, true but not helpful. Some should feel free to upload a patch with messages is they were so inclined.

tim.plunkett’s picture

Added messages.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Docs are much better.

I'm still a bit meh on subform for naming (and the one-word), but don't feel strongly enough about it to hold this up.

Committed 583f49d and pushed to 8.2.x. Thanks!

  • catch committed 03fc204 on 8.2.x
    Issue #2537732 by Xano, tim.plunkett, gnuget, claudiu.cristea, drunken...

  • catch committed 03fc204 on 8.3.x
    Issue #2537732 by Xano, tim.plunkett, gnuget, claudiu.cristea, drunken...

  • catch committed 03fc204 on 8.3.x
    Issue #2537732 by Xano, tim.plunkett, gnuget, claudiu.cristea, drunken...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

geek-merlin’s picture