Problem/Motivation
For certain usecases, especially embeddded forms inside forms, you want to fire submit handlers similar to the way validation can be done on every element.
Proposed resolution
A typical usecase if inline entity forms. You have a form with multiple subentity forms in there. For each, you want to be able to have an easy way to just submit one of the entities inside. #element_submit
allows you to do that.
On top of that, once you have more than one subform for example, you also want to be able to limit the possible elements you want to submit. Just think of a form with two embedded entity forms, each with its own submit button. On every Submit button, you want to just validate and submit their corresponding form elements. Therefore this patch introduces #limit_element_submit
, similar to #limit_validation_errors
.
Remaining tasks
User interface changes
None
API changes
Two new FAPI Element properties
#element_submit
#limit_element_submit
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#120 | 2820359-120.patch | 11.51 KB | _utsavsharma |
#117 | interdiff_110-117.txt | 1.83 KB | berliner |
#117 | 2820359-117.patch | 16.92 KB | berliner |
#117 | 2820359-117-test-only.patch | 10.02 KB | berliner |
#110 | interdiff_108-110.txt | 2.16 KB | Meenakshi_j |
Comments
Comment #2
dawehnerThat's a child of #2820235: Add inline entity form to core as experimental
Here is a test only patch.
Comment #3
jibranShouldn't it be 'element_submit'?
Comment #5
jibranHere is a first stab.
Comment #6
jibrans/element_submitter/element_submit :/
Comment #7
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI think we might be able to backport this to Drupal 7. Tentatively adding the tag for that.
Comment #8
dawehner@jibran
Do you want to give it a try and combine the actual patch and the test one together?
Comment #9
jibranHere we go.
Comment #11
jibranI hope this will be green.
Comment #12
larowlanlooks good to me
Comment #13
phenaproximaWhy does $form_state need to be passed by reference? It's an object, so won't it be passed by reference anyway?
Comment #14
jibranSource: http://php.net/manual/en/language.oop5.references.php
Comment #15
phenaproximaFair enough. Seems harmless to pass it by reference, but based on what I'm reading in the documentation, it looks like passing it as a normal value will have the same effect. So it comes down, apparently, to style.
I'm trying to find things wrong with this patch, but I can't. It looks as elegant and well-tested as it is simple. I'm not sure I'm qualified to mark this RTBC, but I can't wait for it to land!
One tiny question:
Not a big deal at all, but wouldn't just
isset($form[$key]['#element_submit'])
suffice here?Comment #16
tim.plunkettNo, objects are not passed by reference, they are passed as an object identifier. This allows you to manipulate it as though it were a reference, except in one case: you cannot replace the object with a new one.
Internals of FormBuilder/FormValidator/FormSubmitter still explicitly pass &$form_state, including #element_validate. But they shouldn't need to, and we shouldn't add that here without good reason.
So let's please remove the & from $form_state.
Also it doesn't need to be split over so many lines.
Comment #17
jibranI was just following the prior practice.
I disagree, either we remove it from everywhere or just go with the old convention. It is easier to remember this way. I don't want to end up like array_map array_filter scenario.
It is split because I was trying to avoid
PHPCS error.Comment #18
jibranThis is copied from
\Drupal\Core\Form\FormValidator::doValidateForm()
Comment #19
tstoecklerdoSubmitForm()
is not called recursively so this will only call#element_submit
callbacks on top-level elements. This is very non-standard and makes it fairly useless for complex forms.Am I missing something?
Comment #20
dawehner@tstoeckler Great point! I also thin conceptually code which executes submit handlers should be inside
\Drupal\Core\Form\FormSubmitter::executeSubmitHandlers
somehow, as well, this is what the method name says.Comment #21
tim.plunkett#17, I do NOT think we should block this on another issue, and I don't think we should introduce a new API in a known-broken way just because other parts of core are wrong.
I did open #2831312: Remove obsolete pass-by-reference of $form_state though :)
Comment #22
jibranThanks for creating the issue. I'll address #21 and #19 later today.
Comment #23
jibranAddressed #21 and #19.
Comment #24
dawehnerThe test we have should have a
['#element_submit']
deep down the tree, so we ensure it actually works.Comment #25
phenaproximaFound a few nitpicks, as is my wont:
Missing type hint.
$elements is also missing a type hint.
Small nit -- unnecessary blank line.
If $elements[$key] is a truthy scalar, this will pass. The second check should maybe be
is_array($elements[$key])
?Nit: Can this use [] syntax instead?
Comment #27
jibranFeel free to address #25 and #24. I'm not activily working on this anymore.
Comment #30
dawehnerHere is a quick reroll.
Comment #31
jofitz CreditAttribution: jofitz at ComputerMinds commentedAddressed @phenaproxima's nitpicks from #25.
Still need to respond to @dawehner's comment in #24 - can you explain what you mean, please?
Comment #33
jofitz CreditAttribution: jofitz at ComputerMinds commentedIntroduced some fundamental errors there, this looks more promising - not changing the method parameters.
Comment #34
bojanz CreditAttribution: bojanz at Centarro commentedOne thing we'll need to add is introduce some kind of #limit_element_submit array that does for submit what #limit_validation_errors does for validation.
Imagine a form with two inline forms A and B. We might want to submit form B without running #element_submit for form A.
Comment #35
dawehnerI agree once you have more than one submit button on the page you need to be able to limit the element to submit, just like its really common to limit the validation once you have multiple submit buttons. A typical example is inline entity form, you want to be able to just validate and submit the inner entity.
The other question though is: Should this be done here, or is this rather a followup question. For consistency I think it belongs here.
I'm currently reworking the test coverage to be more in line with other parts of the modern form tests ...
Comment #36
dawehnerThis moves the test into its own dedicated class, outside of validation.
Comment #37
dawehnerThis adds now limitation of the submitting.
Comment #40
dawehnerI don't understand the second test failure. This
Comment #41
jibranLet's re-title the issue and expand the summary as well.
Comment #43
tim.plunkettvalidate => submit#limit_validation_errors => #limit_submission_errors
Ignore this!
With #limit_validation_errors, validation still runs, and some errors are ignored.
With #limit_element_submit, we are actually preventing the submit code from running. So the name makes sense!
Comment #44
dawehner@jibran
Can you explain why you added the
$form_id
parameter in #23? If you think about it, we could call the element level submit handlers all the time and then get rid of the parameter in total.Comment #45
jibranI just copied it from
\Drupal\Core\Form\FormValidator::doValidateForm
other than that there is no reason for this.Comment #46
dawehnerThis gets rid of the
$form_id
parameter as its entirely useless here :)I still don't really know why this fails, given it works quite fine on my local system.
Comment #48
jibranThanks for updating the summary. We have to update FAPI doc pages on d.o also ElementInfoManagerInterface::getInfo, Button, and FormElement need docs update.
Comment #49
jibranComment #50
tim.plunkettOh, duh. I looked at ::doValidateForm() and realized why we need $form_id (or any optional param):
::validateForm() calls ::doValidateForm() and passes the $form_id. This triggers a call to ::executeValidateHandlers().
Then ::doValidateForm() itself calls ::doValidateForm(), recursing through the form. But without passing the $form_id through.
This ensures that the form-level handlers only run once per form, not once per element.
This raises the larger question: why differentiate between #validate and #element_validate, or #submit and #element_submit?
Comment #51
dawehnerFor now we have extracted the recursive aspect in its own function:
submitFormElement
. It feels like this is the better pattern. One less condition, a clearer path of execution ...I tried to trace back when
#element_validate
was introduced. It was introduced back in 2007 #138706: FormAPI 3: Ready to rock.Apparently the reasoning was:
I think the receiving different arguments points is a good one. Its actually the same for this
#element_submit
case.Comment #52
dawehnerI think I adapted the necessary bits of documentation now.
Comment #54
dawehnerHa, I forgot something specific ... the
$form_id
bit was all over not calling the submit handlers multiple times.Comment #56
tim.plunkettWhat is $elements?
Please please move that ternary onto it's own line
Bunch of new lines/whitespace
Nit: use ClassName::class
Comment #57
dawehner#56
Thank you for reviewing this patch quickly!
1. Seriously PHP, this reference of a potential undefined variable is kinda crazy
2. Sure
3. Clean it thank you!
4. Fair :)
Comment #58
dawehnerComment #60
dawehner> 80 chars
We need to document the order of execution for these calls, inside out or outside in ...
Comment #61
phenaproximaOkay -- this makes the test pass on my localhost. Hopefully Drupal CI will agree.
Also fixed #60.1.
Comment #65
vasi1186 CreditAttribution: vasi1186 at Amazee Labs commentedThe patch looks pretty good, I also gave it a try and it worked good for me, also the tests pass locally.
I only have some very small things to report, all of them related to coding style (and actually some of them appear also in the other parts of the Drupal core itself).
Nit pick: seems like a type in the comment, it should be 'can also use $form_state'
This line seems to be way bellow 80 characters. The same for the next one.
Another nit pick: seems like a typo: "could be a an form".
Comment #66
vasi1186 CreditAttribution: vasi1186 at Amazee Labs commentedI fixed the small issues from 66, but do not really understand what is needed for 60.2.
Comment #67
vasi1186 CreditAttribution: vasi1186 at Amazee Labs commentedComment #68
phenaproximaTests are still failing, so this still needs work :)
Comment #69
vasi1186 CreditAttribution: vasi1186 at Amazee Labs commented@phenaproxima that's quite strange, don't know why they are failing. I've run the same tests locally and they work just fine... Any ideas?
Comment #70
jofitz CreditAttribution: jofitz commentedI too am unable to obtain the error messages (but I've corrected the coding standards error).
I have gone down the rabbit hole and found that in this case Tests/WebAssert.php:77 calls
ElementNotFoundException($this->session,...
when it should be passing$this->session->getDriver()
. In this file there are multiple examples of both, some were corrected as part of #2961691: Change SYMFONY_DEPRECATIONS_HELPER back to strict, but I'm not sure why all of them weren't. I suspect this bug needs it's own ticket, but I'm not sure I understand it enough to create the ticket!Comment #73
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFixed the failing test and the group name of that test class.
Comment #75
michaellander CreditAttribution: michaellander at Elevated Third commentedI've tested this patch using '#element_submit' on this issue #3037221: Block forms are submitted prematurely. It is glorious. Looking at recent comments here, I think it's good to RTBC.
Comment #76
tstoecklerSomething I don't see being covered in the tests is an element with an
#element_submit
property that also has a child which itself has an#element_submit
property.As far as I can tell it should work in the way that the form will be submitted inside out - which makes sense, I think - but it would be good to have explicit test coverage of that.
Comment #77
PanchoAlso, I think we should reconsider the naming, to make sure it is consistent, at least logically.
In #43, @tim.plunkett raised the question of consistency, however figured out that:
Still, if the counterpart to
#element_validate
is#element_submit
,the counterpart to
#limit_validation_errors
should be#limit_submission
, not#limit_element_submit
.Comment #78
rgpublicYou meant "#limit_submission_errors", I suppose? I'd agree with that...
Comment #79
michaellander CreditAttribution: michaellander at Elevated Third commentedI think they meant
#limit_submission
as they said because to @tim.plunkett's point earlier, we are limiting submission, instead of limiting some errors from showing. Personally, I think#limit_element_submit
is actually the best name for it, though I don't think#limit_submission
is bad either, especially as I don't see us renaming#limit_validation_errors
anytime soon.@tstoeckler I think that's a good idea. From what I can tell, it's submitting as expected through children, but adding test coverage wouldn't hurt.
Comment #80
PanchoIndeed, I suggested
#limit_submission
.Unfortunately, I currently don’t have time to dig into #370537: Allow suppress of form errors (hack to make "more" buttons work properly) (name decision in #171 there), nor into #763376: Not validated form values appear in $form_state, but it seems the latter is a bug and it was originally intended not to submit unvalidated values anyway.
So instead of introducing more properties, we could possibly just fix that bug? On the other hand, the bug could have become a feature over all the years.
Not sure though if the better solution would be this or a toggle/modification of #limit_validation_errors, though.
Comment #81
michaellander CreditAttribution: michaellander at Elevated Third commentedHere is a version that tests a parent
#element_submit
with a child#element_submit
. It does not however test the order in which they are executed.@tstoeckler is this what you had in mind?
Comment #82
dawehnerI'm not sure where, but it would be quite nice to expand some existing documentation, like
/Users/dawehner/www/d8/core/lib/Drupal/Core/Form/FormBuilderInterface.php
to include this additional functionality in the overviewing documentation.There it would be nice to describe when do which function run (inside out or outside in for example).
Comment #83
czigor CreditAttribution: czigor at Centarro commented1. I think the documentation is in its right place in Button, FormElement and ElementInfoManagerInterface. This follows the #element_validation and #limit_validation_errors practice. Added some fixes there.
2. Removed the excessive NULL argument from the recursive submitFormElement() call. (Not sure how the tests passed before though.)
3. Changed text matches to regexp matches in tests so that we test the execution order too.
4. Made the tests a bit more compact by using a universal submit callback for all elements.
Comment #84
czigor CreditAttribution: czigor at Centarro commented5. Also removed some &s from before $form_states.
6. #element_submit docs now say "The execution order in the form array is bottom to top."
I think all raised concerns have been addressed. Let's RTBC this.
Comment #85
bojanz CreditAttribution: bojanz at Centarro commentedWas 60.2 ("document the order of execution for these calls, inside out or outside in") addressed? Doesn't look like it at a glance.
The second sentence that was used for #element_validate, copied here, but is it equally correct? At this point we have form state values that we can use too.
"submit the input" sounds a bit odd, perhaps "submit the element"?
I'd love to get a final +1 from dawehner before RTBC.
Comment #86
czigor CreditAttribution: czigor at Centarro commentedThis is meant to be that part:
+ * The execution order is bottom to top in the form array.
We pass
$element, $form_state, $form
to the #element_submit callbacks, yes. (Don't know why $form is needed, since it's available in $form_state, if that's the question.)Agreed, fixed.
Comment #87
bojanz CreditAttribution: bojanz at Centarro commentedOne additional tricky thing to consider: #limit_validation_errors is an array of #parents, but #limit_element_submit is an array of #array_parents.
Not the same thing. And even our own docs get it wrong in one place:
Comment #88
czigor CreditAttribution: czigor at Centarro commentedFixed docs.
To me #array_parents feels a bit better choice since all levels are always available that way (in case you can't control #tree for some reason). Of course we can't change #limit_validation_errors now. In a follow-up issue we could emphasize this difference.
Comment #90
jhedstromPatch failed to apply in #88.
Comment #91
czigor CreditAttribution: czigor at Centarro commentedReroll.
Comment #93
czigor CreditAttribution: czigor at Centarro commentedComment #95
czigor CreditAttribution: czigor at Centarro commentedTrying to fix tests.
Comment #96
czigor CreditAttribution: czigor at Centarro commentedComment #97
czigor CreditAttribution: czigor at Centarro commentedsudo get fixed now
Comment #99
kpv CreditAttribution: kpv at DrupalBASE commentedComment #100
czigor CreditAttribution: czigor at Centarro commentedComment #102
czigor CreditAttribution: czigor at Centarro commentedComment #106
geek-merlinYes, still needed a lot.
Comment #107
geek-merlinWorking in IEF into that direction. Let's start with an overview and consolidation ticket.
Comment #108
berliner CreditAttribution: berliner commentedJust updated the last patch to apply on Drupal 9.1.10, not sure at what version it started to not properly apply anymore.
Comment #110
Meenakshi_j CreditAttribution: Meenakshi_j at Srijan | A Material+ Company for Drupal India Association commentedFixed the fails of #108
Comment #113
jonathanshawHas the test coverage requested in #76 been added?
Comment #114
geek-merlinThere are tests in the patch.
We need to prove it covers the new code though, with a test-only patch.
(Anti-Mess: If someone can take this simple task, please attach the test-only patch first, then the full patch second, to your comment.)
Comment #117
berliner CreditAttribution: berliner commentedRe-rolled for 10.1 and extracted a test-only patch from #110.
Comment #118
berliner CreditAttribution: berliner commentedComment #119
smustgrave CreditAttribution: smustgrave at Mobomo commentedMay need to be rerolled for 11.x
Also was tagged for change record which still believe is needed.
Comment #120
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedRerolled patch for 11.x