Do not allow form builder functions to return Response objects

We used to have more of these. LanguageDeleteForm::buildForm() seems to be the only usage left.

Needed to update the coverage for sendResponse to use the other call to it now that we're removing this one.

I don't think this is actually possible to prevent. We should have a dozen more failures than we do, which means missing test coverage.

Look at ViewsFormBase::getForm() and UserMultipleCancelConfirm::buildForm() and explain how we're going to fix those.

Instead, we need to try again at allowing forms to *properly* redirect when they are functioning as controllers.
If that means that FormBuilder needs a separate buildFormAsController method that is called from Drupal\Core\Controller\FormController, then so be it.

Here are some more fixes and test coverage. This should fail.
And I haven't touched ViewsFormBase yet.

Made a mistake with that, ViewsFormBase doesn't use _form, it's _content.

In IRC I suggested an extra interface with a dedicated method that forms can implement. If they do, then when used as a controller (only) that is called before buildForm() is, and it can either return a (redirect?) response (in which case buildForm is never called) or null, in which case buildForm() is called. That has the added advantage of separating the "should this form even happen" logic from the form definition itself.

While it may result in a little code duplication, I suspect factoring the code in the form class to separate methods will eliminate most of it, enough that I'm not worried about it.

This needs work and I don't have the time to work on it, but this might help.

Well, we could convert many of those instances to use a custom controller which uses a form controller and set the redirect outside of it.

Here it is without the additional interface.

That's important :)
Plus a 25K patch is much better than a 24.99K patch

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -416,10 +416,8 @@ public function retrieveForm($form_id, FormStateInterface &$form_state) {
+      throw new \UnexpectedValueException('Form builder functions must return arrays');

Replace "Array" with "Form array structure" to be clearer about the data "type" that is expected.

Using _content here is fine by me, too. Although, if we're going to put getForm() on the form class itself would it make sense to standardize that, much as we do create() on controllers? Which... becomes the interface suggestion all over again? :-)

Here's a completely different approach. It doesn't require module authors to do anything differently for the 99.99% use case.

Thanks for the substantive reviews.

1. +++ b/core/modules/language/src/Form/LanguageDeleteForm.php
   @@ -88,13 +88,6 @@ public function getFormId() {
   -    // Warn and redirect user when attempting to delete the default language.
   ...
   -      return new RedirectResponse($url);
   ...
        // Throw a 404 when attempting to delete a non-existing language.
        $languages = language_list();
        if (!isset($languages[$langcode])) {
   

   Suppose access checker should do that!

2. +++ b/core/modules/node/src/Form/DeleteMultiple.php
   @@ -97,9 +97,12 @@ public function getConfirmText() {
        if (empty($this->nodes)) {
   -      return new RedirectResponse($this->getCancelUrl()->setAbsolute()->toString());
   +      $node_temp_store->delete($current_user_id);
   +      throw new AccessDeniedHttpException();
   

   That needs a code comment, also 403 is a strange way to break flow.
   Maybe better to wrap that logic into controller?

3. +++ b/core/modules/simpletest/simpletest.routing.yml
   @@ -17,7 +17,7 @@ simpletest.test_form:
   -    _form: 'Drupal\simpletest\Form\SimpletestResultsForm'
   +    _content: 'Drupal\simpletest\Form\SimpletestResultsForm::getForm'
   
   +++ b/core/modules/simpletest/src/Form/SimpletestResultsForm.php
   @@ -8,19 +8,19 @@
   -class SimpletestResultsForm extends FormBase {
   +class SimpletestResultsForm extends ControllerBase implements FormInterface {
   
   @@ -106,16 +106,22 @@ public function getFormId() {
   +  public function getForm($test_id = NULL) {
   +    if (is_numeric($test_id) && !$results = $this->getResults($test_id)) {
   +      drupal_set_message($this->t('No test results to display.'), 'error');
   
   @@ -252,6 +258,12 @@ public function buildForm(array $form, FormStateInterface $form_state, $test_id
   +  public function validateForm(array &$form, FormStateInterface $form_state) {
   +  }
   

   good trick

4. +++ b/core/modules/system/src/Form/ModulesListConfirmForm.php
   @@ -107,7 +108,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
        // Redirect to the modules list page if the key value store is empty.
        if (!$this->modules) {
   ...
   +      throw new AccessDeniedHttpException();
   

   I think it would be great to add some usable error message here and other similar places.

Let's get some push behind this again. First let's someone reroll it.

One trivial conflict to resolve, lots of merging and auto-merging going on

That it installs is all I can guarentee ... let us see what testbot says

I can see a way to resolve at least some of the broken tests

the function definition of

FormBuilderInterface::submitForm() has changed since this patch was last modified ...

I will fix this up.

This needs the "beta evaluation" criteria added.
I don't see how this should block release.

1. +++ b/core/modules/simpletest/simpletest.routing.yml
   @@ -17,7 +17,7 @@ simpletest.test_form:
   +    _content: 'Drupal\simpletest\Form\SimpletestResultsForm::getForm'
   
   +++ b/core/modules/user/user.routing.yml
   @@ -78,7 +78,7 @@ entity.user_role.edit_permissions_form:
   +    _content: '\Drupal\user\Form\UserMultipleCancelConfirm::getForm'
   

   HEAD now uses _controller instead of _content

2. +++ b/core/modules/system/src/Form/ModulesListConfirmForm.php
   @@ -119,7 +120,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      throw new AccessDeniedHttpException();
   
   +++ b/core/modules/system/src/Form/ModulesUninstallConfirmForm.php
   @@ -133,7 +133,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      throw new AccessDeniedHttpException();
   

   Should this not have a message explaining why access is denied? Is that the right Exception to throw?

Cross-post

Looking at FormBuilderTest

Stepping through the code, the mocks in testSendResponse() expect the dispatch and terminate events to fire. This is the reported error.

Actually what is happening in FormBubmitter::doSubmitForm() a response is set

and then FormBuilder::getForm() throws and error diverting the normal flow of code out of balance.

So this is interlinked with this issue...

#2367555: Deprecate EnforcedResponseException support

It may take sometime to find a common way forward.

Please ignore my comment from #31 .. I will file a separate issue.

From #25

FormBuilder::sendResponse() does not exist anymore. I think this test is obsolete now.

Yes.. wish I had seen that earlier :)

I also fixed the others from #25 all except 6.

This look a better way to control error paths, in the short term the error count should increase .. but not too much!

I typed 'action' when I meant 'actions' so the submit buttons were not deleted, In these corner cases when a malformed/hacked form is submitted.

I have simplified the text I added in #38, and made the message text better. I am providing a sample screenshot to show the response

1) #34.1 as per https://www.drupal.org/node/2378809 now using _controller

2) SimpleResultsForm::getForm - should be on an interface but is not. Just providing an initial annotation,
Something bugs me about it that I cannot articulate - documenting it oftens helps.... the return complex type seems inelegant
[ This is just a note to myself to revisit it. :) ]

UninstallTest now passes ... so this should be back to green.

I thought this logic, from \Drupal\system\Form\ModulesUninstallConfirmForm was being exercised...

    // Cancel is the only option when the module list is empty.
    if (empty($this->modules)) {
      drupal_set_message('There are no modules to uninstall.', 'error');
      $form = parent::buildForm($form, $form_state);
      unset($form['actions']['submit']);
      $form['description']['#markup'] = 'Press cancel to return.';
      return $form;
    }

turns out this has the last word... this is fine by me. ( \Drupal\system\Form\ModulesUninstallForm )

  public function validateForm(array &$form, FormStateInterface $form_state) {
    // Form submitted, but no modules selected.
    if (!array_filter($form_state->getValue('uninstall'))) {
      $form_state->setErrorByName('', $this->t('No modules selected.'));
    }
  }

Added test to cover the new code in \Drupal\system\Form\ModulesUninstallConfirmForm()

Added tests to cover the new code in \Drupal\system\Form\ModulesListConfirmForm.

plus a couple of trivial tweaks.

The point of this issue is that from builders should not return response objects. So, I am adding back the conversions that were removed in the reroll.

Now that 8.0.x is out, we can't add an exception there anymore.
Also I understood the major-ness of removing exit, but now with \Drupal\Core\Form\EnforcedResponse and \Drupal\Core\EventSubscriber\EnforcedFormResponseSubscriber, is this really that major anymore?

Here's the simplest possible stab to deprecate this. Let's see what breaks.

Verbosified message.

Got em all?

+++ b/core/lib/Drupal/Core/Form/FormBase.php
@@ -211,8 +211,13 @@ protected function currentUser() {
+   * @deprecated in Drupal 9.1.x and will be removed before 10.0.0. Throw a
...
+    @trigger_error(__METHOD__ . ' is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Throw a \Drupal\Core\Form\EnforcedResponseException instead. See https://www.drupal.org/node/2363189. Current form ID: ' . $this->getFormId(), E_USER_DEPRECATED);

We'll need to update these to 10.1 and 11.0 now

All the test fails are because we're passing a Url instead of a Response, which I think means all of the Url objects need to wrapped in a RedirectResponse - so maybe its worth adding a new factory method on EnforcedResponseException e.g throw new EnforcedResponseException::redirectToUrl($url)

Updated issue summary

Added the factory method, hopefully this helps get tests greener

Fixing CCF in #81

Fixing CCF

Fixed errors generated in #84. Have changed reference of formRoutes. It may help in error to be solved.

Tests still failing