[TASK] Deprecate extbase StopActionException

There are three possible cases an extbase controller
action can come up with:
a) Return a casual psr-7 response (html, json, ...)
b) Return an extbase specific ForwardResponse to dispatch
   extbase-internally to another action.
c) Have a redirect the client should receive to
   call some other Url.

a) and b) are simple in v11 - the action returns a
PSR-7 ResponseInterface. c) however throws StopActionException
instead, which is caught by extbase Dispatcher and
then returned. This is ugly.

The patch changes this and deprecates the StopActionException.
We can not drop the throw call since that would be breaking,
but we prepare towards a clean v12 solution, which leads to
the sitation that *all* extbase actions return a response.

Resolves: #94351
Releases: master
Change-Id: Ie5a53109959a008ab1666f52d5a81e6e7ba3efdb
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/69498
Tested-by: core-ci <typo3@b13.com>
Tested-by: Daniel Goerz <daniel.goerz@posteo.de>
Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Daniel Goerz <daniel.goerz@posteo.de>
Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>

typo3/sysext/core/Documentation/Changelog/master/Deprecation-94351-ExtextbaseStopActionException.rst

@@ -0,0 +1,60 @@
+.. include:: ../../Includes.txt
+
+=====================================================
+Deprecation: #94351 - ext:extbase StopActionException
+=====================================================
+
+See :issue:`94351`
+
+Description
+===========
+
+To further prepare towards clean PSR-7 Request / Response handling in
+extbase, the extbase internal :php:`TYPO3\CMS\Extbase\Mvc\Exception\StopActionException`
+has been deprecated.
+
+
+Impact
+======
+
+No deprecation is logged, but the :php:`StopActionException` will be
+removed in v12 as breaking change. Extension developers with extbase
+based controllers can prepare in v11 towards this.
+
+
+Affected Installations
+======================
+
+Extensions with extbase controllers that throw :php:`StopActionException` or
+use methods :php:`redirect` or :php:`redirectToUri` from extbase :php:`ActionController`
+are affected.
+
+
+Migration
+=========
+
+As a goal, extbase actions will *always* return a :php:`ResponseInterface`
+in v12. v11 prepares towards this, but still throws the :php:`StopActionException`
+in :php:`redirectToUri`. Developers should prepare towards this, however.
+
+Example before:
+
+.. code-block:: php
+
+   public function fooAction()
+   {
+      $this->redirect('otherAction');
+   }
+
+Example compatible with v10, v11 and v12 - IDE's and static code analyzers
+may complain in v10 and v11, though:
+
+.. code-block:: php
+
+   public function fooAction(): ResponseInterface
+   {
+      // A return is added!
+      return $this->redirect('otherAction');
+   }
+
+.. index:: PHP-API, NotScanned, ext:extbase

typo3/sysext/extbase/Classes/Mvc/Controller/ActionController.php

@@ -20,6 +20,7 @@
 use Psr\Http\Message\ResponseInterface;
 use TYPO3\CMS\Core\Http\HtmlResponse;
 use TYPO3\CMS\Core\Http\PropagateResponseException;
+use TYPO3\CMS\Core\Http\RedirectResponse;
 use TYPO3\CMS\Core\Http\Response;
 use TYPO3\CMS\Core\Http\Stream;
 use TYPO3\CMS\Core\Messaging\AbstractMessage;
@@ -946,19 +947,17 @@ public function forward($actionName, $controllerName = null, $extensionName = nu
      *
      * Redirect will be sent to the client which then performs another request to the new URI.
      *
-     * NOTE: This method only supports web requests and will thrown an exception
-     * if used with other request types.
-     *
      * @param string $actionName Name of the action to forward to
      * @param string|null $controllerName Unqualified object name of the controller to forward to. If not specified, the current controller is used.
      * @param string|null $extensionName Name of the extension containing the controller to forward to. If not specified, the current extension is assumed.
      * @param array|null $arguments Arguments to pass to the target action
      * @param int|null $pageUid Target page uid. If NULL, the current page uid is used
      * @param null $_ (optional) Unused
      * @param int $statusCode (optional) The HTTP status code for the redirect. Default is "303 See Other
-     * @throws StopActionException
+     * @throws StopActionException deprecated since TYPO3 11.0, method will RETURN a Core\Http\RedirectResponse instead of throwing in v12
+     * @todo: ': ResponseInterface' (without ?) in v12 as method return type together with redirectToUri() cleanup
      */
-    protected function redirect($actionName, $controllerName = null, $extensionName = null, array $arguments = null, $pageUid = null, $_ = null, $statusCode = 303)
+    protected function redirect($actionName, $controllerName = null, $extensionName = null, array $arguments = null, $pageUid = null, $_ = null, $statusCode = 303): void
     {
         if ($controllerName === null) {
             $controllerName = $this->request->getControllerName();
@@ -977,24 +976,17 @@ protected function redirect($actionName, $controllerName = null, $extensionName
     /**
      * Redirects the web request to another uri.
      *
-     * NOTE: This method only supports web requests and will thrown an exception if used with other request types.
-     *
      * @param mixed $uri A string representation of a URI
      * @param null $_ (optional) Unused
      * @param int $statusCode (optional) The HTTP status code for the redirect. Default is "303 See Other"
-     * @throws StopActionException
+     * @throws StopActionException deprecated since TYPO3 11.0, will be removed in 12.0
+     * @todo: ': ResponseInterface' (without ?) in v12 as method return type together with redirectToUri() cleanup
      */
-    protected function redirectToUri($uri, $_ = null, $statusCode = 303)
+    protected function redirectToUri($uri, $_ = null, $statusCode = 303): void
     {
         $uri = $this->addBaseUriIfNecessary($uri);
-        $response = new HtmlResponse(
-            '',
-            $statusCode,
-            [
-                'Location' => (string)$uri
-            ]
-        );
-
+        $response = new RedirectResponse($uri, $statusCode);
+        // @deprecated since v11, will be removed in v12. RETURN the response instead. See Dispatcher class, too.
         throw new StopActionException('redirectToUri', 1476045828, null, $response);
     }
 

typo3/sysext/extbase/Classes/Mvc/Dispatcher.php

@@ -18,6 +18,7 @@
 use Psr\Container\ContainerInterface;
 use Psr\EventDispatcher\EventDispatcherInterface;
 use Psr\Http\Message\ResponseInterface;
+use TYPO3\CMS\Core\Http\RedirectResponse;
 use TYPO3\CMS\Core\SingletonInterface;
 use TYPO3\CMS\Extbase\Annotation\IgnoreValidation;
 use TYPO3\CMS\Extbase\Event\Mvc\AfterRequestDispatchedEvent;
@@ -90,8 +91,15 @@ public function dispatch(RequestInterface $request): ResponseInterface
             try {
                 $response = $controller->processRequest($request);
                 if ($response instanceof ForwardResponse) {
+                    // The controller action returned an extbase internal Forward response:
+                    // Another action should be dispatched.
                     $request = static::buildRequestFromCurrentRequestAndForwardResponse($request, $response);
                 }
+                if ($response instanceof RedirectResponse) {
+                    // The controller action returned a core HTTP redirect response.
+                    // Dispatching ends here and response is sent to client.
+                    return $response;
+                }
             } catch (StopActionException $ignoredException) {
                 $response = $ignoredException->getResponse();
             }

typo3/sysext/extbase/Classes/Mvc/Exception/StopActionException.php

@@ -28,6 +28,10 @@
  * continues dispatching the request or returns control to the request handler.
  *
  * See the Action Controller's forward() and redirectToUri() methods for more information.
+ *
+ * @deprecated since v11, will be removed in v12. This action shouldn't be thrown anymore:
+ * Actions that extbase-internally forward to another action should RETURN Extbase\Http\ForwardResponse
+ * instead. Actions that initiate a client redirect should RETURN a Core\Http\RedirectResponse instead.
  */
 class StopActionException extends Exception
 {
@@ -38,6 +42,9 @@ class StopActionException extends Exception
 
     public function __construct($message = '', $code = 0, \Throwable $previous = null, ResponseInterface $response = null)
     {
+        // @deprecated since v11, will be removed in v12. Can not trigger_error() here since
+        // extbase ActionController still has to use this exception for b/w compatibility.
+        // See the usages of this exception when dropping in v12.
         $this->response = $response ?? new Response();
         parent::__construct($message, $code, $previous);
     }

typo3/sysext/extensionmanager/Classes/Controller/UploadExtensionFileController.php

@@ -98,7 +98,7 @@ public function formAction(): ResponseInterface
      * Extract an uploaded file and install the matching extension
      *
      * @param bool $overwrite Overwrite existing extension if TRUE
-     * @throws \TYPO3\CMS\Extbase\Mvc\Exception\StopActionException
+     * @throws StopActionException @deprecated since v11, will be removed in v12
      */
     public function extractAction($overwrite = false)
     {
@@ -145,17 +145,20 @@ public function extractAction($overwrite = false)
                         FlashMessage::OK
                     );
                 } else {
+                    // @deprecated since v11, change to return $this->redirect()
                     $this->redirect('unresolvedDependencies', 'List', null, ['extensionKey' => $extensionKey]);
                 }
             }
         } catch (StopActionException $exception) {
+            // @deprecated since v11, will be removed in v12: redirect() will no longer throw in v12, drop this catch block
             throw $exception;
         } catch (InvalidFileException $exception) {
             $this->addFlashMessage($exception->getMessage(), '', FlashMessage::ERROR);
         } catch (\Exception $exception) {
             $this->removeExtensionAndRestoreFromBackup($fileName);
             $this->addFlashMessage($exception->getMessage(), '', FlashMessage::ERROR);
         }
+        // @deprecated since v11, change to return $this->redirect()
         $this->redirect('index', 'List', null, [
             self::TRIGGER_RefreshModuleMenu => true,
             self::TRIGGER_RefreshTopbar => true

typo3/sysext/form/Classes/Domain/Finishers/RedirectFinisher.php

@@ -17,10 +17,9 @@
 
 namespace TYPO3\CMS\Form\Domain\Finishers;
 
-use Psr\Http\Message\ResponseInterface;
-use TYPO3\CMS\Core\Http\Stream;
+use TYPO3\CMS\Core\Http\PropagateResponseException;
+use TYPO3\CMS\Core\Http\RedirectResponse;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
-use TYPO3\CMS\Extbase\Mvc\Exception\StopActionException;
 use TYPO3\CMS\Extbase\Mvc\Web\Routing\UriBuilder;
 
 /**
@@ -37,7 +36,6 @@ class RedirectFinisher extends AbstractFinisher
     protected $defaultOptions = [
         'pageUid' => 1,
         'additionalParameters' => '',
-        'delay' => 0,
         'statusCode' => 303,
     ];
 
@@ -46,11 +44,6 @@ class RedirectFinisher extends AbstractFinisher
      */
     protected $request;
 
-    /**
-     * @var ResponseInterface
-     */
-    protected $response;
-
     /**
      * @var \TYPO3\CMS\Extbase\Mvc\Web\Routing\UriBuilder
      */
@@ -64,7 +57,6 @@ protected function executeInternal()
     {
         $formRuntime = $this->finisherContext->getFormRuntime();
         $this->request = $formRuntime->getRequest();
-        $this->response = $formRuntime->getResponse();
         $this->uriBuilder = GeneralUtility::makeInstance(UriBuilder::class);
         $this->uriBuilder->setRequest($this->request);
 
@@ -73,11 +65,10 @@ protected function executeInternal()
         $additionalParameters = $this->parseOption('additionalParameters');
         $additionalParameters = is_string($additionalParameters) ? $additionalParameters : '';
         $additionalParameters = '&' . ltrim($additionalParameters, '&');
-        $delay = (int)$this->parseOption('delay');
         $statusCode = (int)$this->parseOption('statusCode');
 
         $this->finisherContext->cancel();
-        $this->redirect($pageUid, $additionalParameters, $delay, $statusCode);
+        $this->redirect($pageUid, $additionalParameters, $statusCode);
     }
 
     /**
@@ -90,18 +81,17 @@ protected function executeInternal()
      *
      * @param int $pageUid Target page uid. If NULL, the current page uid is used
      * @param string $additionalParameters
-     * @param int $delay (optional) The delay in seconds. Default is no delay.
      * @param int $statusCode (optional) The HTTP status code for the redirect. Default is "303 See Other
      * @see forward()
      */
-    protected function redirect(int $pageUid = 1, string $additionalParameters = '', int $delay = 0, int $statusCode = 303)
+    protected function redirect(int $pageUid = 1, string $additionalParameters = '', int $statusCode = 303)
     {
         $typolinkConfiguration = [
             'parameter' => $pageUid,
             'additionalParams' => $additionalParameters,
         ];
         $redirectUri = $this->getTypoScriptFrontendController()->cObj->typoLink_URL($typolinkConfiguration);
-        $this->redirectToUri($redirectUri, $delay, $statusCode);
+        $this->redirectToUri($redirectUri, $statusCode);
     }
 
     /**
@@ -110,25 +100,18 @@ protected function redirect(int $pageUid = 1, string $additionalParameters = '',
      * NOTE: This method only supports web requests and will thrown an exception if used with other request types.
      *
      * @param string $uri A string representation of a URI
-     * @param int $delay (optional) The delay in seconds. Default is no delay.
      * @param int $statusCode (optional) The HTTP status code for the redirect. Default is "303 See Other
-     * @throws StopActionException
+     * @throws PropagateResponseException
      */
-    protected function redirectToUri(string $uri, int $delay = 0, int $statusCode = 303)
+    protected function redirectToUri(string $uri, int $statusCode = 303)
     {
         $uri = $this->addBaseUriIfNecessary($uri);
-        $escapedUri = htmlentities($uri, ENT_QUOTES, 'utf-8');
-
-        $body = new Stream('php://temp', 'r+');
-        $body->write('<html><head><meta http-equiv="refresh" content="' . (int)$delay . ';url=' . $escapedUri . '"/></head></html>');
-        $body->rewind();
-
-        $this->response = $this->response
-            ->withBody($body)
-            ->withStatus($statusCode)
-            ->withHeader('Location', (string)$uri)
-        ;
-        throw new StopActionException('redirectToUri', 1477070964, null, $this->response);
+        $response = new RedirectResponse($uri, $statusCode);
+        // End processing and dispatching by throwing a PropagateResponseException with our response.
+        // @todo: Should be changed to *return* a response instead, but this requires the ContentObjectRender
+        // @todo: to deal with responses instead of strings, if the form is used in a fluid template rendered by the
+        // @todo: FluidTemplateContentObject and the extbase bootstrap isn't used.
+        throw new PropagateResponseException($response, 1477070964);
     }
 
     /**

typo3/sysext/form/Classes/ViewHelpers/RenderViewHelper.php

@@ -111,6 +111,13 @@ public static function renderStatic(array $arguments, \Closure $renderChildrenCl
         try {
             return $form->render();
         } catch (StopActionException $exception) {
+            // @deprecated since v11, will be removed in v12: StopActionException is deprecated, drop this catch block.
+            // RedirectFinisher for throws a PropagateResponseException instead which bubbles up into Middleware.
+            trigger_error(
+                'Throwing StopActionException is deprecated. If really needed, throw a (internal) PropagateResponseException'
+                . ' instead, for now. Note this is subject to change.',
+                E_USER_DEPRECATED
+            );
             return $exception->getResponse()->getBody()->getContents();
         }
     }

typo3/sysext/form/Tests/Unit/Domain/Finishers/RedirectFinisherTest.php

@@ -18,9 +18,9 @@
 namespace TYPO3\CMS\Form\Tests\Unit\Domain\Finishers;
 
 use Prophecy\Argument;
+use TYPO3\CMS\Core\Http\PropagateResponseException;
 use TYPO3\CMS\Core\Http\Response;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
-use TYPO3\CMS\Extbase\Mvc\Exception\StopActionException;
 use TYPO3\CMS\Extbase\Mvc\Request;
 use TYPO3\CMS\Extbase\Mvc\Web\Routing\UriBuilder;
 use TYPO3\CMS\Extbase\Object\Exception;
@@ -84,7 +84,7 @@ public function pageUidOptionForFinisherAcceptsVariousPageRepresentations($pageU
         try {
             $redirectFinisherMock->execute($finisherContextProphecy->reveal());
             self::fail('RedirectFinisher did not throw expected exception.');
-        } /** @noinspection PhpRedundantCatchClauseInspection */ catch (StopActionException $e) {
+        } /** @noinspection PhpRedundantCatchClauseInspection */ catch (PropagateResponseException $e) {
             $response = $e->getResponse();
             self::assertSame($uriPrefix . $expectedPage, $response->getHeader('Location')[0]);
         }