#2310255: [meta] Remove ArrayAccess from FormState expanded the FormStateInterface, and also removed support for get/set/has to modify internal properties. Additionally, setIfNotExists has been removed.

We have 3 places in core that could use a SubFormState (BlockBase, BlockForm, ImageEffectFormBase), and I can imagine that pattern in many contrib modules.
Have you thought about opening a core issue? I could do that once your version is updated...

CommentFileSizeAuthor
#2 2338837-sapi-2.patch24.3 KBtim.plunkett

Comments

drunken monkey’s picture

Status: Active » Fixed

Thanks for posting this!
I had already seen the changes, though, and fixed them locally. I was just working on fixing all the other newly introduced test failures (there really changed a lot in the last one or two weeks, every time I adapt to a Core change there's a new one breaking the module) before pushing, but I now just committed and pushed anyways. I think the SubFormState class is already correct.

I didn't think this could be handy for Core, too, but if it is that would of course be great. Please feel free to open a Core issue for it, yes. Thanks!
(If it turns out there still were changes necessary to the class, I'll just post in that issue then.)

tim.plunkett’s picture

Status: Fixed » Needs review
StatusFileSize
new24.3 KB

Great! I'll post here if I open an issue (taking a look at how useful it'd be now).

One note, seeing your comment on SubFormState::setGroups.

That's just for vertical tabs, see \Drupal\Core\Render\Element\RenderElement::processGroup(), I don't think you have to worry.

In the meantime, here's a patch for how I would have written this without SubFormState, aka how core works.

Are you sure that the sub-form_states you're storing in validate and retrieving in submit will be up to date? Won't other validate handlers that run after this one that happen to update values cause it to get out of sync?

xano’s picture

I may have misunderstood this issue and the conversation on IRC wrongly, but why is this done? If this is to get a plugin form's values from a global form state, one can easily do $plugin_form_values = NestedArray($element[‘#parents’], $form_state->getValues());, as long as the parent form passes on the result of buildConfigurationForm() as the $form parameter to validateConfigurationForm() and submitConfigurationForm(). I don't think we should be faking the form state, as that allows for more problems to occur.

xano’s picture

drunken monkey’s picture

Thanks for all your comments here!
It didn't occur to me to use #parents for that, but yes, that would of course also be an easy solution, without introducing a new concept. Since my solution doesn't do anything about the problem with #states, etc., developers would also need to be familiar with using #parents for this kind of nesting problem anyways.
What I liked about my approach is that it becomes trivial for plugin authors to deal with the form state part of the problem – but of course it introduces a new source of errors (even though it seems to work flawlessly at the moment), so maybe that's not worth it and plugin authors just have to work around this problem themselves. Also, if someone does use #parents, it might be confusing that the parents don't correspond to the nesting they see in the form state.

In the meantime, here's a patch for how I would have written this without SubFormState, aka how core works.

Ah, so Core does also pass a "fake" form state? Then I don't understand Xano's first comment – it seems this is already done in core, isn't it, just not with a dedicated class?
Then my reaons for using one instead of just faking a form state with the right values, is that all the normal functionality of a form state will be preserved this way, too. In tim.plunkett's patch example, getting or changing the submitted form values works fine – but nothing else (storing arbitrary data to pass between the different methods, setting/getting the "rebuild" flag, retrieving the triggering element, …) does. The SubFormState uses the Proxy pattern and some references to solve this problem in a single place, giving the plugin form the same control and functionality in the form state as a normal form would have. Even if this isn't needed in Core at the moment, I think it would be dangerous to not do this as it could easily be needed by some (e.g., block) plugin in contrib which wouldn't really have any option to circumvent the problem (as far as I can see). It also doesn't seem like we currently document anywhere that the form states passed to the plugin's form methods will only support the values-related functionality – a potential source of confusion/frustration.

Are you sure that the sub-form_states you're storing in validate and retrieving in submit will be up to date? Won't other validate handlers that run after this one that happen to update values cause it to get out of sync?

SubFormState uses references to the original form state's properties, so it should always be up-to-date. Also, other validate functions usually won't touch the values of a plugin's form.

@ chx: That's partly related, but seems to concern a different aspect of the sub-form problem – one that the SubFormState class approach doesn't manage to solve. So, in any case, interesting issue, thanks for linking it!

drunken monkey’s picture

Status: Needs review » Fixed

I guess this is now fixed? (Though I had to update more methods a few days back, again …)
It seems like it didn't make it into Core, but it was of course rather unlikely to begin with.

Status: Fixed » Closed (fixed)

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

xano’s picture