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.
Comment | File | Size | Author |
---|---|---|---|
#126 | interdiff.txt | 1.11 KB | tim.plunkett |
#126 | 2537732-plugin-126.patch | 103.17 KB | tim.plunkett |
#123 | 2537732-123--subform_states.patch | 103.07 KB | drunken monkey |
Comments
Comment #1
XanoComment #2
XanoComment #3
EclipseGc CreditAttribution: EclipseGc commentedThrough 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
Comment #4
XanoMoving 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:
FormStateInterface
, but most of that interface won't be usable by plugin forms.Using
NestedArray
:PluginFormInterface::validateConfigurationForm()
andPluginFormInterface::submitConfigurationForm()
.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.
Comment #5
tim.plunkettNeeds docs updates still, but how about this?
Comment #6
jhodgdonUm, is that the right patch in #5? Doesn't seem to be docs or related to the previous patch.
Comment #7
tim.plunkettSince #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.
Comment #8
jhodgdonAh. Well we need a new issue summary/title then.
Comment #9
tim.plunkettDid my best to retitle and update the IS.
Comment #10
EclipseGc CreditAttribution: EclipseGc commentedI 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
Comment #11
XanoI 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: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.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 multipleFormSate
instances per request, and those static methods are meant to preserve global state, I'm not sure they should live on that class.Comment #12
XanoThis implements the idea coined in #11. The patch does not contain tests or implementations of this functionality yet.
Comment #14
tim.plunkettMeh. 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.
Comment #15
XanoIf 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 dedicatedFormStateManager
class to keep track of global form validation errors.Aye, aye!
Comment #16
tim.plunkettHmm I guess this makes some sense. I still think it's out of scope.
This isn't actually true, it can be either. Use | to separate the typehints please!
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...
Beautiful!
Comment #17
tim.plunkettI 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.
Comment #19
tim.plunkettWent 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.
Comment #20
XanoThanks, this looks really good!
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.Comment #21
tim.plunkettWe *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:
$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?!
Comment #23
tim.plunkettThat 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.
Comment #24
XanoYes. 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.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 defaultFormState
implementation, soFormStateInterface
changes in ~8.1.0 releases don't seem out of the question.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.
:D
Comment #25
XanoThis 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.Comment #27
XanoComment #28
tim.plunkettI 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?
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.
What about testing the new InvalidArgumentException in getSubFormProperty()?
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.
Comment #29
XanoRe-roll. I will address the feedback from #28 in another patch.
Comment #31
XanoI updated the method names to reflect the names of the array keys.
I updated the patch so
PluginFormInterface
always receives the subform instead of the parent form.You already did that in
::testGetValuesBroken()
, but I updated the@covers
annotations to reflect this.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?
Comment #33
EclipseGc CreditAttribution: EclipseGc commentedWe'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
Comment #34
XanoTim'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.
Comment #35
Xano#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.
Comment #36
borisson_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?
Creates a new instance of SubFormState or extending class for a subform.
would be a better comment, I think.+1
Comment #39
XanoComment #40
XanoAnother thing we can do now is scope
FormStateInterface::set()
and::get()
, so there can be no clashes between parent and embedded forms.Comment #41
bojanz CreditAttribution: bojanz at Centarro commentedFrom 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).
Comment #42
XanoHow exactly?
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.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 keepSubFormState
(which contains the logic this is all about) more readable, not 'obscured' by the simple decorations.Comment #43
XanoAdding a duplicate issue.
Comment #45
tim.plunkettI think the "other methods on $form_state are completely ignored" thing is a convincingly bad problem.
Comment #46
XanoShould we do this, or not?
Comment #47
tim.plunkettStraight reroll.
Comment #49
tim.plunkettReroll.
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #51
tim.plunkettReviewing my patch would help.
Comment #52
dsnopekLooking at #4:
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:Or am I missing something?
Comment #53
Xano@dsnopek: That's a very old comment, from before the current approach even existed. I don't remember what I meant by that.
Comment #54
dsnopek@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.Comment #55
davidwbarratt CreditAttribution: davidwbarratt at Golf Channel commentedI applied the patch from #49 and I still was not able to save block config in a Plugin field.
Comment #56
XanoRe-roll. I'm also working on some test coverage.
Comment #57
davidwbarratt CreditAttribution: davidwbarratt at Golf Channel commentedKicking off the test bot. :)
Comment #58
XanoNow 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 forFormState
, because we'll need to override::getValues()
inSubFormState
in order to retrieve the parent form state's values and extract the subset. If we keepFormStateValuesTrait::getValues()
abstract, we move the current implementation toFormState
, and we use the trait inFormStateDecoratorBase
Comment #59
XanoComment #60
Xano#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.
Comment #61
XanoWe 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.Comment #62
XanoThis patch breaks BC, because of the introduction of
FormStateInterface::getSubformState()
. That must be removed. We can useSubformState::createForSubform()
instead.Comment #63
chx CreditAttribution: chx commentedAdding a method to this interface... Do you expect people will write their own FormState objects not extending FormState?
Comment #64
tim.plunkettSearch API has done it, to work around this exact issue. That's where we got the idea for this implementation.
Comment #65
mallezieComment #66
Xano@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 :)Comment #67
claudiu.cristeaNice!
Some notes, most of them nits.
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
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" :)
Remove one empty line.
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:
or something like this?
Newline.
These tests have no assertions. I think PHP Unit will mark them as unsafe.
Short array syntax [].
Missing doc-blocks for data providers.
EDIT: And, yes, the BC break
Comment #68
XanoThanks for the review! Those are good points, and I'll incorporate them in the next patch.
Adding them to
FormState
, but notFormStateInterface
prevents a BC break, but is a little silly, because it makesFormState
depend onSubFormState
. We should move the factory (if we want to keep any) toSubFormState
, and make it depend onFormStateInterface
, so our classes are decoupled. Mental note: subforms are not restricted to plugins.Comment #69
XanoI moved this code back to
FormState
, becauseFormStateValuesTrait
made too many assumptions about variable storage. These decorators are tricky enough without a misleading and unused$values
property.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 matchFormStateInterface::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 toSubformState
with as few disruptions as possible.Comment #70
XanoComment #71
XanoComment #72
XanoComment #73
gnugetWhen 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.
Comment #74
claudiu.cristeaI want to verify #73, so I added the tests from #2649746: Block config form does not show errors from blockValidate and #2645784: Error validation messages produced by image effect plugins not shown.
Comment #75
gnugetThe #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.
Comment #77
XanoAh, of course. We've got to decorate the error methods as well.
Comment #78
XanoI 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.Comment #79
XanoThis patch improves test coverage, documentation, variable naming, and fixes the bug that the test failure from #75 exposed (thanks, @gnuget!).
Comment #80
XanoFear my copy/paste skillz!
Comment #81
borisson_Why do we need to have this method? This doesn't add any value as far as I can see.
There should be only one blank line here.
The summary of the docblock should be only one line.
This method should get a docblock.
The method description could be one line.
/s/subject/system
Comment #82
Manjit.SinghIs it be the part of 8.2.x branch ?
Comment #83
Manjit.SinghIs it be the part if 8.2.x branch ?
Comment #84
tim.plunkettWell 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.
Comment #85
Manjit.SinghComment #86
XanoAs long as this is a bug report, this is allowed for 8.1.x.
Comment #87
XanoI 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.
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.
Comment #89
tim.plunkettIt's an API addition, no chance for 8.0.x
But I'll let a committer tell you that.
Comment #90
bojanz CreditAttribution: bojanz at Centarro commentedAgreed, on march 26th the chances of this coming near 8.0.x are long gone.
Comment #91
XanoI 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 toFormStateDecoratorBase
.Comment #95
XanoComment #96
borisson_The other questions I had about this were answered by @Xano at drupalcamp spain.
I think these need an actual description as well.
These should be removed. There's more @file blocks in this patch.
Comment #97
XanoThanks @borisson_ for the review, and to @bojanz for pointing out (on IRC) that
FormStateInterface::getUserInput()
andFormStateInterface::setUserInput()
would need to be decorated as well. This patch addresses the given feedback, and adds additional test coverage forSubformState::getValue()
andSubformState::getValues()
to verify that they return by reference.Comment #99
XanoI'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:SubFormState::getUserInput()
returns the global input, which prevents us from getting the subform user input directly.public function &getUserInput()
becomespublic function &getUserInput()
. This should work, becauseFormStateInterface::setUserInput()
exists as well, but breaks BC.Comment #100
XanoWe concluded we cannot properly decorate the user input methods, so let's do it without that for now.
Comment #102
XanoComment #104
XanoRe-roll.
Comment #105
XanoThis ensures embedded condition forms keep working like they do now, and effectively solves #2647464: Condition plugin configuration forms depend on their parent forms.
Comment #106
XanoAny other concerns about this approach?
Comment #107
tim.plunkettWhy do we need this?
And this?
These are the only confusing changes to implementation.
Perhaps just a comment would help.
Nice forward thinking. Does a double nested form_state have coverage in this patch?
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!
Comment #108
bojanz CreditAttribution: bojanz at Centarro commentedIt 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?
Comment #109
XanoThanks for reviewing this patch again!
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.
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 inBlockForm
to indicate this should be removed in Drupal 9.Yes, see
\Drupal\Tests\Core\Form\SubformStateTest::testGetCompleteFormStateWithParentSubform()
.I standardized on subform earlier on in this issue, and corrected the sub form occurrences you pointed out.
Comment #110
XanoWe crossposted, so I wasn't able to implement your feedback in #109. Here it is.
Comment #111
bojanz CreditAttribution: bojanz at Centarro commentedThe changes in #109 look good.
I can't really parse the comment added in #110:
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?
Comment #112
XanoYou'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.Comment #113
bojanz CreditAttribution: bojanz at Centarro commentedI 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.
Comment #114
XanoSince the thought of never-resolved arbitrary suggestions sends shivers down my spine, I agree with @bojanz it's best to avoid those.
Comment #115
jonathanshawComment #116
tim.plunkett+1 for RTBC.
Comment #117
xjmI mentioned to @tim.plunkett that a CR could help with this issue.
Comment #118
tim.plunkettAttempted reroll after #2763157: Allow plugins to provide multiple forms went in.
Comment #119
XanoChange record added at https://www.drupal.org/node/2774077.
Comment #120
catchHad 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.
Don't love nested ternaries - have to scroll a full screen width in dreditor to get to the end of this one.
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.
not $parentForm?
Not $subForm?
Had a think about different ways to do this, but they were all worse. So yeah fair enough.
Again this is a bit scant.
Why the !empty?
Comment #121
catchCNW 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.
Comment #122
XanoI'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.
Comment #123
drunken monkeyThis should address all complaints from #120, except this one:
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.
Comment #124
tim.plunkettThanks @drunken monkey
Comment #125
alexpottThis 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.
Comment #126
tim.plunkettAdded messages.
Comment #127
catchDocs 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!
Comment #132
geek-merlinFollowup #2983321: Reveal identity of subforms or widgets