Remove exit() from FormBuilder

While I'd love to see some improvements to FormBuilder, I'd like to note that it handles the request/response flow internally, and fires the terminate event just fine.

Here is an idea how we could do it.

Reopening/rescoping this issue because the other one (#1497162: Eliminate the use of exit in core) has no solution for the FormBuilder (which is rather critical after the introduction of stackphp).

Reroll.

Actually remove exit calls from FormBuilder (and also remove dependency on kernel), fix unit-test, rise priority of exception subscriber.

Oh, fun. Exception thrown while exception is handled. This basically means that it is necessary to special case anything where something is rendered outside of HttpKernel::handle().

Remaining test-fails seem to have something to do with twig. I have no clue about this area.

Also examine every previous exception accessible via Exception::getPrevious(), this should resolve the twig issue.

Symfony HttpKernel::handleException() sets the response status code to 500 unless the exception handled implements the HttpExceptionInterface. Therefore rebase the new HttpResponseException onto the generic Symfony HttpException and make sure the response status code is propagated properly.

More exception handler fun: If a custom error-page is configured, the HTTP status of the original error will potentially overrule the one of HttpResponseException. A possible solution is that CustomPageExceptionHtmlSubscriber::makeSubrequest() only enforces the status of the subrequest-response if it is smaller than 300.

Awesome work!!

1. +++ b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php
   @@ -133,7 +133,9 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $path, $s
   +        if ($response->getStatusCode() < 300) {
   

   What about using $response->isSuccessFul()? It would be great to document why we do this here.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionResponseSubscriber.php
   @@ -0,0 +1,43 @@
   +    while ($exception) {
   +      if ($exception instanceof HttpResponseException) {
   +        // Setting the response stops the event propagation.
   +        $event->setResponse($exception->getResponse());
   +        break;
   +      }
   +
   +      $exception = $exception->getPrevious();
   

   Can we document why we iterate over the backtrace?

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -1072,17 +1061,11 @@ protected function buttonWasClicked($element, FormStateInterface &$form_state) {
      protected function sendResponse(Response $response) {
   -    $request = $this->requestStack->getCurrentRequest();
   -    $event = new FilterResponseEvent($this->httpKernel, $request, HttpKernelInterface::MASTER_REQUEST, $response);
   -
   -    $this->eventDispatcher->dispatch(KernelEvents::RESPONSE, $event);
   -    // Prepare and send the response.
   -    $event->getResponse()
   -      ->prepare($request)
   -      ->send();
   -    $this->httpKernel->terminate($request, $response);
   

   We should also document why we throw an exception here. This is not obvious to be honest.

Re #18.1: This piece of code is in fact brittle. When a form decides to return a response with status=200, then the this will not work.

Before cleaning up the code, I'd like to try something different: Pass around a wrapped response and just unpack it from within a response-filter in the main-request. Like that it should be possible to properly propagate a response generated by a form in a subrequest to the main response.

Let's be explicit about the use-case and rename WrappedHttpResponse to EnforcedResponse and HttpResponseException to EnforcedResponseException and move them to the Drupal\Core\Form namespace.

Also rename ExceptionResponseSubscriber to EnforcedFormResponseSubscriber.

Polish the documentation and revert #13. That is not necessary anymore because of the response-wrapping and filtering introduced in #19.

This is using exceptions as a flow-of-control tool, not as an error handling tool. That's very bad form. :-) That's why we went with the exit() in the first place, because that was less evil than using exceptions for program flow control.

Are we certain there's no alternative? If we are forced to go this route, it should be HEAVILY documented that it's a hack around fundamental design flaws in FAPI to discourage people from using it as an example.

Maybe instead, you could explain the "fundamental design flaws", and your suggestion for what you would have designed instead. Then maybe we could implement that.

But just complaining again and again about unenumerated flaws is not helping anyone.

It's been over a year so I don't recall the details. In general terms, the problem is that FAPI has no way to return a value after processing a submitted form. I don't really recall who was working on that with me at the time, unfortunately, but the form build process had no way to return a response back out of the controller. It would have required heavily rewriting the form processing pipeline which no one had the stomach for at the time. It was very insistent on calling drupal_goto() as its last step. Turning that into a dedicated process exit point was the best we could do to ensure that response and terminate listeners still got called.

That said, with all of the refactoring of FAPI that you've done in the last few months it's possible that we could now find a way to return a RedirectResponse back from a processed form and use the standard pipeline, which would solve the issue here quite nicely. I've not been inside FAPI in a long time, though, so I don't know if that's just wishful thinking on my part. You've been in there more than I have so you can probably answer that far better than I could. But that's the original problem that prompted the dedicated process exit point.

See #1668866-128: Replace drupal_goto() with RedirectResponse and next..the problem is with nested forms with redirects.

I agree with Crell and #23, exceptions are used for error handling, this looks uglier than the exit call.

What is the problem with leaving it as is? Or just the fact that we want grep to return 0 results for exit()? nvm, just saw this is about middlewares

Yes, the problem is not within the Form component, it's how we have forms embedded in all kinds of places blocks and parts of pages and so on. Those can't pass the redirect through and they would have to add an is_array()/is response check everywhere.

The only way that could change I guess is if we'd route form submissions differently and directly to the form, similarly how we do it for standard ajax responses.

In order to fix the control flow of the form API one very fundamental step needs to be done first: Separate form-submission from rendering, i.e. the action of each form needs to point to a dedicated route.

Regrettably the call to exit() breaks stack middlewares.

Note that resolving this issue is necessary in order to #2229145: Register symfony session components in the DIC and inject the session service into the request object.

This will also block a http less test base class, ie kernel only tests of controllers etc

I keep thinking there's an issue open for having form submits go to a dedicated route, but I can't find it. Not opening now - hoping someone else knows where that issue is. Let's open one if there's not.

I know we discussed that back in Munich. Also the possibility of using a request listener that checked for a POST request (and various other things) regardless of the path being used. That would also be a viable option, I think.

Also the possibility of using a request listener that checked for a POST request

That does not work because the API currently allows form builder methods to return a response (yes, when forms are rendered, not only when they are submitted). We could try to remove that "feature". Possibly affected code (with some false positives):

$ git grep -lE '(implements|extends).*Form|namespace.*Form' | xargs git grep -l Response | grep -v -i test
core/lib/Drupal/Core/Form/FormBuilder.php
core/lib/Drupal/Core/Form/FormBuilderInterface.php
core/lib/Drupal/Core/Form/FormState.php
core/lib/Drupal/Core/Form/FormStateInterface.php
core/lib/Drupal/Core/Form/FormSubmitter.php
core/lib/Drupal/Core/Form/FormSubmitterInterface.php
core/modules/config_translation/src/FormElement/DateFormat.php
core/modules/editor/src/Form/EditorImageDialog.php
core/modules/editor/src/Form/EditorLinkDialog.php
core/modules/file/src/Controller/FileWidgetAjaxController.php
core/modules/language/src/Form/LanguageDeleteForm.php
core/modules/locale/src/Form/ExportForm.php
core/modules/node/src/Form/DeleteMultiple.php
core/modules/system/core.api.php
core/modules/system/src/Form/CronForm.php
core/modules/system/src/Form/DateFormatFormBase.php
core/modules/system/src/Form/ModulesListConfirmForm.php
core/modules/system/src/Form/ModulesUninstallConfirmForm.php
core/modules/user/src/Form/UserMultipleCancelConfirm.php
core/modules/user/src/RegisterForm.php
core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
core/modules/views_ui/src/Form/Ajax/ViewsFormInterface.php
core/modules/views_ui/src/ViewEditForm.php

Going a bit through history (mostly #1668866: Replace drupal_goto() with RedirectResponse), turned up:

• #72:
  

  I brainstormed a bit with Berdir in IRC. Suggestion: Add a _drupal_register_redirect(RedirectResponse $response) function. Document it as a last-resort and for internal use only. Then add a response listener (or maybe just view listener?) that checks for the presence of a registered redirect response and swaps that in as the response object to send.

• #110:
  

  but swapping response events leads to some nice results, eg lost messages from dsm..not to mention that redirection happens after drupal has theme the entire page.

• #131:
  

  Instead, we're going to accept that there will be some fugly to make this work but it will at least be contained to FAPI. [...] To that end, drupal_goto() will be removed and replaced with _drupal_form_send_response(). _dfsr() will:

  * Take a response object passed to it by FAPI (usually either a RedirectResponse or BinaryFileResponse)
  * Call the kernel.response event on it directly via Drupal::
  * prepare() and send() the response
  * Call Drupal::service('http_kernel')->terminate()
  * PHP exit;

With #1668866-131: Replace drupal_goto() with RedirectResponse it was finally possible to make the form API work without drupal_goto. The problem we have now is that since then stack-middleware has entered the picture and I cannot think of any other way than throwing an exception to make this work with the current approach - except perhaps rewriting the form API entirely.

Regrettably I was not able to find the issue mentioned by @catch in #31.

@znerol so I'd personally be fine with us having that issue open, as major/critical and fixing post-beta. It'd be an API change but it may make this issue considerably easier to solve, as well as being a performance improvement on form submits.

Problem is that this blocks #2229145: Register symfony session components in the DIC and inject the session service into the request object. I'm not sure about the priority over there, but given that it introduces the Symfony session service (i.e. the facade which anything operating on the session should use) I'd love to have this in place for the beta.

Related: #2248297: Ensure routes are rebuilt when install modules

Reroll after #2248297: Ensure routes are rebuilt when install modules.

Would anyone be willing to Assign this issue to themselves. It is a Critical so needs an owner. I'm a Base system maintainer and doing triage on critical bugs in this component.

I might get tomatoes thrown at me, but if the Critical problem here is breaking StackPHP, then we could consider rolling back StackPHP. Having forms POST to dedicated routes is a huge change at this point in the release cycle.

Filed #2363189: Do not allow form builder functions to return Response objects.

Would anyone be willing to Assign this issue to themselves. It is a Critical so needs an owner. I'm a Base system maintainer and doing triage on critical bugs in this component.

Is that owner nicety documented somewhere? https://www.drupal.org/node/2172049 says that assigning issues is a bad thing.

I might get tomatoes thrown at me, but if the Critical problem here is breaking StackPHP, then we could consider rolling back StackPHP

Well, the critical is that exit() is called from FormBuilder, no matter whether there is stack or not. Yes, calling exit() is also problematic for stackphp,
but it is not the problematic space.

#39: Regardless of stack or not calling the DrupalKernel and exit directly from some class is not a good idea as the DrupalKernel is an internal API used by the bootstrap process, not by modules.

#41: Assigning is probably not the right thing, but having one or several "owners" in the issue description that are both responsible and can be asked questions if someone wants to work on it, probably is.

#40: Lets postpone this shortly on #2362517: Improve default 403/404 exception HTML subscriber: don't duplicate the page render pipeline, use the routing system — add system.403 and system.404 routes and also see what really needs to happen here.

For just sending a 302 or such surely no response object but a more higher level API is needed, for all other cases I don't know why form builder would need to return a response early and the issue summary does not make this clear.

Note @tim.plunkett independently came to the same solution (i.e. use exceptions for the control flow) in #2363189-20: Do not allow form builder functions to return Response objects. @Crell: Regrettably I cannot see any realistic alternatives to a) continue here and replace exit with exceptions or b) do what @moshe weitzman proposes in #39 and remove support for stacked kernels.

I think the proposal in #32 is rather difficult to implement. The problem is that some form builder functions accept additional arguments. As a result it would be necessary to unconditionally cache any form along with its arguments (except those forms with a dedicated route). Even if we succeed to implement that, we'd still need a clean way to solve #2363189: Do not allow form builder functions to return Response objects.

Can we get an issue summary update of why the FormBuilder needs to exit early in the first place?

Together with a proposed resolution?

In addition, the problem statement needs clarity. What is broken due to this bug. Note #1 where Tim says that the request/response is handled internally and the Terminate event is fired. If this about code cleanliness, it should not be Critical.

Worth nothing that things calling exit also limits our ability for KernelTestBase to test pages/controller output without http (aka BrowserKit driver for Mink) - which is a big step towards improving the performance of our test-framework

Good point, @larowlan. Now I see that you mentioned that in #30 as well. Added to IS.

We don't have a suggestion better than the exception so I propose we proceed with latest patch - #38.

See also #2363189-20: Do not allow form builder functions to return Response objects, which is a more targeted approach and doesn't modify DefaultExceptionHtmlSubscriber directly, keeping it within the Form subsystem.

@tim.plunkett: #2363189-20: Do not allow form builder functions to return Response objects does not seem to resolve exit() during form submission.

Reroll after #2362517: Improve default 403/404 exception HTML subscriber: don't duplicate the page render pipeline, use the routing system — add system.403 and system.404 routes. I think the changes introduced to the exception handlers are not necessary anymore (#10).

#50: Glad to hear that :)

One small correction: #50 still does change the default exception handler, but not anymore the default HTML exception handler.

+++ b/core/lib/Drupal/Core/EventSubscriber/EnforcedFormResponseSubscriber.php
@@ -0,0 +1,53 @@
+  /**
+   * Unwraps an enforced response.
+   */
+  public function onKernelResponse(FilterResponseEvent $event) {
+    $response = $event->getResponse();
+    if ($response instanceof EnforcedResponse && $event->getRequestType() === HttpKernelInterface::MASTER_REQUEST) {
+      $event->setResponse($response->getResponse());
+    }
+  }

I tried to understand the issue, but I did not get it.

Why is this chunk needed at all?

Can't we just set directly the response we want?

See #18 and #19. We need to restore the wrapped response in the response subscriber of the master request. Otherwise it is impossible/brittle to detect an exception thrown by a form rendered in a subrequest (e.g. 403/404).

This can happen if, e.g. the login block is displayed on the access-denied page. Upon form submission a redirect is issued but the status code will be overridden by the exception handler (i.e. becomes 403 instead of 302). It is safer to restore the whole response after the exception was handled and that's what the hunk does.

Thanks for the explanation, do you feel the patch is ready?

#23 is not yet addressed:

If we are forced to go this route, it should be HEAVILY documented that it's a hack around fundamental design flaws in FAPI to discourage people from using it as an example.

Adding tests (interdiff equals TEST-ONLY patch).

Attempting to address #23.

This looks really great to me now! And I assume this is ready.

Has additional tests, fixes the issue, makes sense, feedback addressed => RTBC

+1 for this

+++ b/core/modules/system/src/Tests/Form/ResponseTest.php
@@ -0,0 +1,52 @@
+    $edit = [
+      'content' => $this->randomString(),
+      'status' => 418,
+    ];

418 my favourite HTTP status code!

Some minor nits found whilst reviewing. It would also be nice if the issue summary reflected the current solution and pointed to caveat documented in the patch. And I guess we should be opening a postponed 9.x critical to address this

fundamental design flaws in FAPI

1. +++ b/core/lib/Drupal/Core/Form/EnforcedResponse.php
   @@ -0,0 +1,78 @@
   + * desires to explicitely set a response object. Exception handlers capable of
   

   explicitly

2. +++ b/core/lib/Drupal/Core/Form/EnforcedResponse.php
   @@ -0,0 +1,78 @@
   +  /**
   +   * Constructs a new enforced response from the given exception.
   +   *
   +   * Note that it is necessary to traverse the exception chain when searching
   +   * for an enforced response. Otherwise it would be impossible to find an
   +   * exception thrown from within a twig template.
   +   *
   +   * @param Exception $e
   +   *   The exception where the enforced response is to be extracted from.
   +   */
   +  public static function createFromException(\Exception $e) {
   +    while ($e) {
   +      if ($e instanceof EnforcedResponseException) {
   +        return new static($e->getResponse());
   

   It should be @param \Exception $e

   Also missing @return. Perhaps something like

   +   * @return \Drupal\Core\Form\EnforcedResponse|NULL
   +   *   The enforced response or NULL if the exception chain does not contain a
   +   *   \Drupal\Core\Form\EnforcedResponseException exception.
   

3. +++ b/core/lib/Drupal/Core/Form/EnforcedResponseException.php
   @@ -0,0 +1,52 @@
   +   * @param Exception $previous
   

   Should be @param \Exception $previous

4. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -1081,24 +1078,6 @@ protected function buttonWasClicked($element, FormStateInterface &$form_state) {
   -    $event = new FilterResponseEvent($this->kernel, $request, HttpKernelInterface::MASTER_REQUEST, $response);
   ...
   -    $this->eventDispatcher->dispatch(KernelEvents::RESPONSE, $event);
   

   Can remove

   use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
   use Symfony\Component\HttpKernel\HttpKernelInterface;
   use Symfony\Component\HttpKernel\KernelEvents;
   

   from this class.

Thanks. Updated issue summary and patch.

we should be opening a postponed 9.x critical to address this

Filed #2367555: Deprecate EnforcedResponseException support

Interdiff looks great to me, back to RTBC

@znerol thanks for the fixes. This issue addresses a critical bug and is allowed per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?. Committed dda70f7 and pushed to 8.0.x. Thanks!