submitForm() is not called when custom form #submit handler is set

This should work fine.
Can you provide some working example code to help reproduce?

I have also tested this by alter core theme i.e seven and it is working fine for me. As tim.plunkett said, share some code with us, that would help to understand the exact scenario of this.

Okay I can reproduce. I'll work on writing a test for this.

There are two problems here.
First, you should be using $form['#submit'][] = 'starkers_submit_settings';

Using the button level #submit explicitly skips the generic form submission handler, and that is true in D7 as well.

But, using the top-level #submit is broken just for the theme-specific alters in theme-settings.php, because it runs *before* the regular form builder, not alongside the other alters.

Okay, this wasn't too bad.

If you add $form['#submit'] or $form['#validate'] in your build form (which the theme-settings.php tacks onto), it currently suppresses the regular form handlers.
I don't think this is desired.

You can still suppress them by having button level submit handlers instead.

Makes sense.

If you add $form['#submit'] or $form['#validate'] in your build form (which the theme-settings.php tacks onto), it currently suppresses the regular form handlers.
I don't think this is desired.

Hm. This has been a known/desired behavior and design choice of Form API in the past. We should look up and study why.

Can we re-implement the test as either DUTB or perhaps even phpunit, please? The functionality that is being changed here is not related to a theme at all — it's bare metal FormBuilder processing.

One related issue, and #38349 is the original Form API patch that introduced the #validate/#submit properties.

From #38349-6: new workflow for form validation/execution:

a module could simply add an element to the #validate array via hook_form_alter if it wished to perform additional validation, or it could redeclare the entire #validate array if it wanted to override any existing validation

It is true that we're performing such overrides via button-level handlers in the meantime. However, there's one major pitfall:

The node entity form force-executes the form-level handlers.

Or perhaps even all entity forms in D8? Or has that code been refactored/removed in D8? — In any case, we need to ensure that the claim of "use button-level handlers to override everything" is actually valid and works in all cases. Otherwise, this change would result in non-overridable handlers for (some) forms.

Marking needs work at least for the test — can't see why that can't be a phpunit test (method).

The functionality that is being changed here is not related to a theme at all — it's bare metal FormBuilder processing.

You're misunderstanding the issue.
This has nothing to do with entity forms, or even really form builder processing itself.

It is specifically about ThemeSettingsForm.

Please read ThemeSettingsForm::buildForm, at the bottom.

Yes, the fix included might be a bit heavy handed, but phpunit is not going to be able to test this flow properly (require_once and function_exists).

Forgot to mention that I do support this API change.

I also agree that theme settings form needs better test coverage + ideally some redesign. However, for this particular issue + change, the involvement of a theme just happens to be part of the symptom, but that is irrelevant for the actual fix/change. We really need to become strict(er) on that, because that's how HEAD has ended up with lots of duplicate/nonsense test coverage (in ugh, webtests).

Attached patch re-implements the test as DUTB (since @tim.plunkett was skeptical whether phpunit would work).

I didn't touch the functional change; only applied it after writing the DUTB test.

Removing my test coverage is pretty rude of you.
I understand your disdain for webtests, but I wrote it because that's how this issue was discovered, and ThemeSettingsForm is VERY different from how other forms work.

Please restore the test you removed.

The patch in #12 worked for me on MAYO 8.x-1.x-dev. It's on a core alpha-11 install. Didn't test extensively, but was sure happy to find the patch.

This is the RTBC patch from before, but with sun's added tests, which I've reviewed.

#2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form added a workaround for this issue, and alongside this fix, it causes the submit handlers to run twice.
Removing that.

Sorry for removing those other changes - they felt off-topic for this issue.

Adjusting issue title, as this pertains to form-level handlers, not button-level handlers. The behavior of button-level handlers should not be affected by this change. (Or did I miss something?)

This will need a change notice (although I think that only a few modules will be affected).

I can't see why we need a change notice here - but if we do one needs to exist before a commit.

While not really documented, it is a known fact that a custom #validate and/or #submit handler causes the "default" handlers to no longer get assigned automatically.

Before this change, you had to manually specify the default #validate/#submit handler(s) in case your form constructor defined (additional) custom handlers.

After this change, you need to remove any manually specified default handlers, because they would be invoked twice otherwise - as exemplified by #18.

A duplicate #validate handler is most often (but not always) a big deal, but a duplicate #submit handler breaks things badly.

This only applies to form constructors that specify additional handlers on top of the default handlers.

Lastly, this change implies that a form constructor is no longer able to omit the default handlers by explicitly setting #validate/#submit handlers to different values. (However, a form alter hook and everything else running after prepareForm() is still able to do so.)

While it's a bit obscure, I think #21 is worth putting in a change notice.

Sleeping over this...

The proposed API change here represents ~50% of the following older issue, which proposes to swap the invocation order of form construction functions under the hood:

#1452922: Run drupal_prepare_form() before the form constructor instead of after

If the form constructor gets a fully prepared $form injected already, then most of the magically determined callbacks can be introspected and manipulated in a more controlled way by your code:

You can var_dump($form) and you'll see that #submit will invoke e.g. $this::submitForm(). In turn, you know whether you want to $form['#submit'][] or array_unshift($form['#submit'], ...).

The same applies to #validate, #theme, #attributes, and everything else that is being set up in FormBuilder::prepareForm().

I don't see any commits involved with this issue and yet I don't see the problem after Pull of today's Drupal 8 from git.

Duplicate of #2268761: Remove support for function-based forms

This issue also persists in configuration forms of Drupal 8 core modules which extend ConfigFormBase class. At this point even Drupal 8 core modules do not play well together as described in #2355053: ConfigFormBase::buildForm() is missing a default submit handler.

This means that there's no way to gracefully add additional submit handlers to Drupal 8 configuration forms because any $form['actions']['#submit'][] = 'MYMODULE_form_submit'; code will disable ConfigFormBase::submitForm() method execution.

That is by design. If #submit handlers are set on a specific submit element, then they override the default.

Use the top-level $form['#submit'][] to add additional and still have the default implementation called.

@Berdir, sorry, I hate adding, effectively, a "+1" post, but this saved me. Thanks!