[TASK] Deprecate extbase StopActionException
authorChristian Kuhn <lolli@schwarzbu.ch>
Tue, 15 Jun 2021 13:26:32 +0000 (15:26 +0200)
committerChristian Kuhn <lolli@schwarzbu.ch>
Tue, 15 Jun 2021 18:12:43 +0000 (20:12 +0200)
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 [new file with mode: 0644]
typo3/sysext/extbase/Classes/Mvc/Controller/ActionController.php
typo3/sysext/extbase/Classes/Mvc/Dispatcher.php
typo3/sysext/extbase/Classes/Mvc/Exception/StopActionException.php
typo3/sysext/extensionmanager/Classes/Controller/UploadExtensionFileController.php
typo3/sysext/form/Classes/Domain/Finishers/RedirectFinisher.php
typo3/sysext/form/Classes/ViewHelpers/RenderViewHelper.php
typo3/sysext/form/Tests/Unit/Domain/Finishers/RedirectFinisherTest.php

diff --git a/typo3/sysext/core/Documentation/Changelog/master/Deprecation-94351-ExtextbaseStopActionException.rst b/typo3/sysext/core/Documentation/Changelog/master/Deprecation-94351-ExtextbaseStopActionException.rst
new file mode 100644 (file)
index 0000000..8f91824
--- /dev/null
@@ -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
index 9a597ce..46cfa6e 100644 (file)
@@ -20,6 +20,7 @@ use Psr\Http\Message\ResponseFactoryInterface;
 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,9 +947,6 @@ abstract class ActionController implements ControllerInterface
      *
      * 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.
@@ -956,9 +954,10 @@ abstract class ActionController implements ControllerInterface
      * @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 @@ abstract class ActionController implements ControllerInterface
     /**
      * 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);
     }
 
index 094e2c2..abc8c64 100644 (file)
@@ -18,6 +18,7 @@ namespace TYPO3\CMS\Extbase\Mvc;
 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 @@ class Dispatcher implements SingletonInterface
             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();
             }
index abc7b75..5d0ac11 100644 (file)
@@ -28,6 +28,10 @@ use TYPO3\CMS\Extbase\Mvc\Exception;
  * 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);
     }
index cc2edf5..4371ab4 100644 (file)
@@ -98,7 +98,7 @@ class UploadExtensionFileController extends AbstractController
      * 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,10 +145,12 @@ class UploadExtensionFileController extends AbstractController
                         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);
@@ -156,6 +158,7 @@ class UploadExtensionFileController extends AbstractController
             $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
index 22eaf66..f1c3b91 100644 (file)
@@ -17,10 +17,9 @@ declare(strict_types=1);
 
 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 @@ class RedirectFinisher extends AbstractFinisher
     {
         $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 @@ class RedirectFinisher extends AbstractFinisher
         $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 @@ class RedirectFinisher extends AbstractFinisher
      *
      * @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 @@ class RedirectFinisher extends AbstractFinisher
      * 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);
     }
 
     /**
index 5e91edf..92dfded 100644 (file)
@@ -111,6 +111,13 @@ class RenderViewHelper extends AbstractViewHelper
         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();
         }
     }
index a9f71d9..f7358a4 100644 (file)
@@ -18,9 +18,9 @@ declare(strict_types=1);
 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 @@ class RedirectFinisherTest extends UnitTestCase
         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]);
         }