Add #element_submit and #limit_element_submit just like #element_validate and #limit_validation_errors

+++ b/core/tests/Drupal/Tests/Core/Form/FormSubmitterTest.php
@@ -44,6 +44,33 @@ protected function setUp() {
+      '#element_submit' => [[$mock, 'element_validate']],

Shouldn't it be 'element_submit'?

Here is a first stab.

+++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
@@ -47,6 +48,14 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
+      if (isset($form[$key]) && $form[$key] && isset($form[$key]['#element_submitter'])) {

s/element_submitter/element_submit :/

I think we might be able to backport this to Drupal 7. Tentatively adding the tag for that.

Here we go.

I hope this will be green.

looks good to me

+++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
@@ -47,6 +48,18 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
+            &$form_state,

Why does $form_state need to be passed by reference? It's an object, so won't it be passed by reference anyway?

One of the key-points of PHP 5 OOP that is often mentioned is that "objects are passed by references by default". This is not completely true.

Source: http://php.net/manual/en/language.oop5.references.php

Fair 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:

+++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
@@ -47,6 +48,18 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
+      if (isset($form[$key]) && $form[$key] && isset($form[$key]['#element_submit'])) {

Not a big deal at all, but wouldn't just isset($form[$key]['#element_submit']) suffice here?

No, 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.

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.

I was just following the prior practice.

So let's please remove the & from $form_state.

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.

Also it doesn't need to be split over so many lines.

It is split because I was trying to avoid 'If the line declaring an array spans longer than 80 characters, each element should be broken into its own line' PHPCS error.

Not a big deal at all, but wouldn't just isset($form[$key]['#element_submit']) suffice here?

This is copied from \Drupal\Core\Form\FormValidator::doValidateForm()

+++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
@@ -2,6 +2,7 @@
@@ -47,6 +48,18 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {

@@ -47,6 +48,18 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
+    foreach (Element::children($form) as $key) {

doSubmitForm() 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?

#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 :)

Thanks for creating the issue. I'll address #21 and #19 later today.

Addressed #21 and #19.

Found a few nitpicks, as is my wont:

1. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -98,6 +85,47 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
   +   * @param $elements
   

   Missing type hint.

2. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -98,6 +85,47 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
   +  protected function submitFormElement(&$elements, FormStateInterface &$form_state, $form_id = NULL) {
   

   $elements is also missing a type hint.

3. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -98,6 +85,47 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
   +
   +    // Recurse through all children.
   

   Small nit -- unnecessary blank line.

4. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -98,6 +85,47 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
   +      if (isset($elements[$key]) && $elements[$key]) {
   

   If $elements[$key] is a truthy scalar, this will pass. The second check should maybe be is_array($elements[$key])?

5. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -98,6 +85,47 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
   +        call_user_func_array($form_state->prepareCallback($callback), array(&$elements, $form_state, &$complete_form));
   

   Nit: Can this use [] syntax instead?

Feel free to address #25 and #24. I'm not activily working on this anymore.

Addressed @phenaproxima's nitpicks from #25.

Still need to respond to @dawehner's comment in #24 - can you explain what you mean, please?

Introduced some fundamental errors there, this looks more promising - not changing the method parameters.

One 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.

Let's re-title the issue and expand the summary as well.

validate => 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!

I just copied it from \Drupal\Core\Form\FormValidator::doValidateForm other than that there is no reason for this.

Thanks for updating the summary. We have to update FAPI doc pages on d.o also ElementInfoManagerInterface::getInfo, Button, and FormElement need docs update.

+++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
@@ -85,6 +86,64 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
+  protected function submitFormElement(array &$elements, FormStateInterface &$form_state, $limit_element_submit = []) {
...
+        $this->submitFormElement($elements[$key], $form_state, NULL, $limit_element_submit);
...
+    $this->executeSubmitHandlers($elements, $form_state);

Oh, 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?

1. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -46,9 +47,10 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
   -    $this->executeSubmitHandlers($form, $form_state);
   ...
   +    $this->executeSubmitHandlers($elements, $form_state);
   

   What is $elements?

2. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -46,9 +47,10 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
   +    $this->submitFormElement($form, $form_state, isset($triggering_element['#limit_element_submit']) && $triggering_element['#limit_element_submit'] !== FALSE ? $triggering_element['#limit_element_submit'] : []);
   

   Please please move that ternary onto it's own line

3. +++ b/core/modules/system/tests/src/Functional/Form/ElementSubmitTest.php
   @@ -0,0 +1,59 @@
   +
   +  ¶
   +
   

   Bunch of new lines/whitespace

4. +++ b/core/tests/Drupal/Tests/Core/Form/FormSubmitterTest.php
   @@ -44,6 +44,39 @@ protected function setUp() {
   +    $form_submitter = $this->getMockBuilder('Drupal\Core\Form\FormSubmitter')
   ...
   +      ->with($this->isType('array'), $this->isInstanceOf('Drupal\Core\Form\FormStateInterface'), $this->isType('array'));
   

   Nit: use ClassName::class

Okay -- this makes the test pass on my localhost. Hopefully Drupal CI will agree.

Also fixed #60.1.

The 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).

1. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -85,6 +88,62 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
   +   *   handlers can also $form_state to pass information on to submit handlers.
   

   Nit pick: seems like a type in the comment, it should be 'can also use $form_state'

2. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -85,6 +88,62 @@ public function doSubmitForm(&$form, FormStateInterface &$form_state) {
   +   *   This technique is useful when validation requires file parsing,
   

   This line seems to be way bellow 80 characters. The same for the next one.

3. +++ b/core/lib/Drupal/Core/Render/Element/Button.php
   @@ -15,6 +15,9 @@
   + *   #element_submit executions. A typical usecase could be a an form with
   

   Another nit pick: seems like a typo: "could be a an form".

I fixed the small issues from 66, but do not really understand what is needed for 60.2.

Tests are still failing, so this still needs work :)

@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?

I 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!

Fixed the failing test and the group name of that test class.

I'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.

Something 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.

Also, 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:

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.

Still, if the counterpart to #element_validate is #element_submit,
the counterpart to #limit_validation_errors should be #limit_submission, not #limit_element_submit.

Still, if the counterpart to #element_validate is #element_submit,
the counterpart to #limit_validation_errors should be #limit_submission, not #limit_element_submit.

You meant "#limit_submission_errors", I suppose? I'd agree with that...

I 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.

Indeed, 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.

Here 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?

1. 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.

5. 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.

Was 60.2 ("document the order of execution for these calls, inside out or outside in") addressed? Doesn't look like it at a glance.

+    // Call any element-specific submit handlers. These must act on the element
+    // #value data.

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.

+ * - #element_submit: (array) Array of callables or function names, which
+ *   are called to submit the input. Arguments: $element, $form_state, $form.
+ *   The execution order in the form array is bottom to top.

"submit the input" sounds a bit odd, perhaps "submit the element"?

I'd love to get a final +1 from dawehner before RTBC.

1. Was 60.2 ("document the order of execution for these calls, inside out or outside in") addressed?

   This is meant to be that part:
   + * The execution order is bottom to top in the form array.

2. The second sentence that was used for #element_validate, copied here, but is it equally correct?

   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.)

3. "submit the input" sounds a bit odd, perhaps "submit the element"?

   Agreed, fixed.

One 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:

+   * @param array $limit_element_submit
+   *   An array of form element #parents arrays to limit element level
+   *   submitting to.

Fixed 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.

Patch failed to apply in #88.

Reroll.

Trying to fix tests.

sudo get fixed now

Yes, still needed a lot.

Working in IEF into that direction. Let's start with an overview and consolidation ticket.

Just updated the last patch to apply on Drupal 9.1.10, not sure at what version it started to not properly apply anymore.

Fixed the fails of #108

Has the test coverage requested in #76 been added?

There 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.)

Re-rolled for 10.1 and extracted a test-only patch from #110.

May need to be rerolled for 11.x

Also was tagged for change record which still believe is needed.

Rerolled patch for 11.x