diff --git a/config/v12/typo3-120.php b/config/v12/typo3-120.php index 2011f2c40..d6ef5092c 100644 --- a/config/v12/typo3-120.php +++ b/config/v12/typo3-120.php @@ -8,6 +8,7 @@ use Ssch\TYPO3Rector\CodeQuality\General\RenameClassMapAliasRector; use Ssch\TYPO3Rector\TYPO312\v0\AddMethodToWidgetInterfaceClassesRector; use Ssch\TYPO3Rector\TYPO312\v0\ChangeExtbaseValidatorsRector; +use Ssch\TYPO3Rector\TYPO312\v0\ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector; use Ssch\TYPO3Rector\TYPO312\v0\ImplementSiteLanguageAwareInterfaceRector; use Ssch\TYPO3Rector\TYPO312\v0\MigrateBackendModuleRegistrationRector; use Ssch\TYPO3Rector\TYPO312\v0\MigrateContentObjectRendererGetTypoLinkUrlRector; @@ -171,4 +172,5 @@ $rectorConfig->rule(MigrateFetchToFetchAssociativeRector::class); $rectorConfig->rule(MigrateBackendModuleRegistrationRector::class); $rectorConfig->rule(MigrateContentObjectRendererGetTypoLinkUrlRector::class); + $rectorConfig->rule(ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector::class); }; diff --git a/rules/TYPO312/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector.php b/rules/TYPO312/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector.php new file mode 100644 index 000000000..212928b7b --- /dev/null +++ b/rules/TYPO312/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector.php @@ -0,0 +1,240 @@ +> + */ + public function getNodeTypes(): array + { + return [ClassMethod::class]; + } + + public function getRuleDefinition(): RuleDefinition + { + return new RuleDefinition('Extbase controller actions with redirects must return ResponseInterface', [ + new ConfiguredCodeSample( + <<<'CODE_SAMPLE' +use TYPO3\CMS\Extbase\Mvc\Controller\ActionController; + +class MyController extends ActionController +{ + public function someAction() + { + $this->redirect('foo', 'bar'); + } +} +CODE_SAMPLE + , + <<<'CODE_SAMPLE' +use Psr\Http\Message\ResponseInterface; +use TYPO3\CMS\Extbase\Mvc\Controller\ActionController; + +class MyController extends ActionController +{ + public function someAction(): ResponseInterface + { + return $this->redirect('foo', 'bar'); + } +} +CODE_SAMPLE + , + [ + 'redirect_methods' => ['myRedirectMethod'], + ] + ), + ]); + } + + /** + * @param ClassMethod $node + */ + public function refactor(Node $node): ?Node + { + if ($this->shouldSkip($node)) { + return null; + } + + $this->traverseNodesWithCallable($node, function (Node $node) { + if ($node instanceof Class_ || $node instanceof Function_ || $node instanceof Closure) { + return NodeTraverser::DONT_TRAVERSE_CURRENT_AND_CHILDREN; + } + + if (! $node instanceof Return_) { + return null; + } + + $responseObjectType = new ObjectType('Psr\\Http\\Message\\ResponseInterface'); + + if ($node->expr !== null && $this->isObjectType($node->expr, $responseObjectType)) { + return null; + } + + $returnCallExpression = $node->expr; + + if ($returnCallExpression !== null && $this->isObjectType( + $returnCallExpression, + new ObjectType('Psr\Http\Message\ResponseInterface') + )) { + return null; + } + + if ($returnCallExpression instanceof FuncCall && $this->isName( + $returnCallExpression->name, + 'json_encode' + )) { + return new Return_($this->nodeFactory->createMethodCall( + 'this', + 'jsonResponse', + [$returnCallExpression] + )); + } + + // avoid duplication + $args = $node->expr instanceof MethodCall && $this->isName($node->expr->name, 'htmlResponse') ? [] : [ + $node->expr, + ]; + + return new Return_($this->createHtmlResponseMethodCall($args)); + }); + + $node->returnType = new FullyQualified('Psr\Http\Message\ResponseInterface'); + + $statements = $node->stmts; + $lastStatement = null; + + if (is_array($statements)) { + $lastStatement = array_pop($statements); + } + + if (! $lastStatement instanceof Return_) { + $node->stmts[] = new Return_($this->createHtmlResponseMethodCall([])); + } + + return $node; + } + + private function shouldSkip(ClassMethod $classMethod): bool + { + if ($classMethod->returnType !== null + && $this->isObjectType($classMethod->returnType, new ObjectType('Psr\\Http\\Message\\ResponseInterface')) + ) { + return true; + } + + if (! $this->nodeTypeResolver->isMethodStaticCallOrClassMethodObjectType( + $classMethod, + new ObjectType('TYPO3\CMS\Extbase\Mvc\Controller\ActionController') + )) { + return true; + } + + if (! $classMethod->isPublic()) { + return true; + } + + if ($classMethod->isAbstract()) { + return true; + } + + $methodName = $this->getName($classMethod->name); + + if ($methodName === null) { + return true; + } + + if (! \str_ends_with($methodName, 'Action')) { + return true; + } + + if (\str_starts_with($methodName, 'initialize')) { + return true; + } + + if ($classMethod->stmts === null) { + return false; + } + + $statements = $classMethod->stmts; + $lastStatement = array_pop($statements); + + if ($lastStatement === null) { + return false; + } + + if ($this->lastStatementIsExitCall($lastStatement)) { + return true; + } + + if ($this->lastStatementIsForwardCall($lastStatement)) { + return true; + } + + return $this->hasExceptionCall($lastStatement); + } + + private function lastStatementIsExitCall(Node $lastStatement): bool + { + return $lastStatement instanceof Expression && $lastStatement->expr instanceof Exit_; + } + + private function hasExceptionCall(Node $lastStatement): bool + { + if (! $lastStatement instanceof Throw_) { + return false; + } + + $propagateResponseException = new ObjectType('TYPO3\CMS\Core\Http\PropagateResponseException'); + + return $this->getType($lastStatement->expr) + ->isSuperTypeOf($propagateResponseException) + ->yes(); + } + + private function lastStatementIsForwardCall(Node $lastStatement): bool + { + if (! $lastStatement instanceof Expression) { + return false; + } + + if (! ($lastStatement->expr instanceof MethodCall)) { + return false; + } + + return $this->isName($lastStatement->expr->name, 'forward'); + } + + /** + * @param mixed[] $args + */ + private function createHtmlResponseMethodCall(array $args): MethodCall + { + return $this->nodeFactory->createMethodCall('this', 'htmlResponse', $args); + } +} diff --git a/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRectorTest.php b/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRectorTest.php new file mode 100644 index 000000000..fe55d7b4e --- /dev/null +++ b/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRectorTest.php @@ -0,0 +1,31 @@ +doTestFile($filePath); + } + + /** + * @return \Iterator> + */ + public static function provideData(): \Iterator + { + return self::yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + public function provideConfigFilePath(): string + { + return __DIR__ . '/config/configured_rule.php'; + } +} diff --git a/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/Fixture/extbase_controller_actions_return_response_interface.php.inc b/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/Fixture/extbase_controller_actions_return_response_interface.php.inc new file mode 100644 index 000000000..a060cb409 --- /dev/null +++ b/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/Fixture/extbase_controller_actions_return_response_interface.php.inc @@ -0,0 +1,212 @@ +view->assign('foo', 'bar'); + } + + public function fooAction(): ResponseInterface + { + $this->view->assign('foo', 'bar'); + return $this->htmlResponse(); + } + + public function initializeSomeAction() + { + // Do something here + } + + protected function createDemandObjectFromSettings($settings) + { + } + + public function detailAction(bool $showError): ?string + { + if ($showError) { + return 'Error forever'; + } + + $this->view->assign('foo', 'bar'); + } + + public function infoAction() + { + $response = null; + try { + $this->forward('foo'); + } catch (\Exception $e) { + HttpUtility::setResponseCode(HttpUtility::HTTP_STATUS_400); + } + + return json_encode($response); + } + + public function deleteDiagramAction() + { + $this->forward('diagramlist'); + } + + public function exportAction() + { + echo 'Export something here'; + exit(); + } + + public function handleExpiredRegistrationsAction() + { + $this->redirect('list'); + } + + public function imageDeleteAction() + { + $this->redirectToUri('foo'); + } + + public function additionalRedirectMethodToSkiAction() + { + $this->additionalRedirectMethod(); + } + + private function additionalRedirectMethod() + { + } + + public function someAction(): ResponseInterface + { + return new JsonResponse(); + } + + public function someActionWithSwitchStatement(string $format): ResponseInterface + { + return JsonResponse::fromArray(); + } + + public function redirectAction() + { + return new RedirectResponse('https://example.com'); + } +} + +?> +----- +view->assign('foo', 'bar'); + return $this->htmlResponse(); + } + + public function fooAction(): ResponseInterface + { + $this->view->assign('foo', 'bar'); + return $this->htmlResponse(); + } + + public function initializeSomeAction() + { + // Do something here + } + + protected function createDemandObjectFromSettings($settings) + { + } + + public function detailAction(bool $showError): ResponseInterface + { + if ($showError) { + return $this->htmlResponse('Error forever'); + } + + $this->view->assign('foo', 'bar'); + return $this->htmlResponse(); + } + + public function infoAction(): ResponseInterface + { + $response = null; + try { + return new ForwardResponse('foo'); + } catch (\Exception $e) { + HttpUtility::setResponseCode(HttpUtility::HTTP_STATUS_400); + } + + return $this->jsonResponse(json_encode($response)); + } + + public function deleteDiagramAction(): ResponseInterface + { + return new ForwardResponse('diagramlist'); + } + + public function exportAction() + { + echo 'Export something here'; + exit(); + } + + public function handleExpiredRegistrationsAction(): ResponseInterface + { + return $this->redirect('list'); + } + + public function imageDeleteAction(): ResponseInterface + { + return $this->redirectToUri('foo'); + } + + public function additionalRedirectMethodToSkiAction() + { + $this->additionalRedirectMethod(); + } + + private function additionalRedirectMethod() + { + } + + public function someAction(): ResponseInterface + { + return new JsonResponse(); + } + + public function someActionWithSwitchStatement(string $format): ResponseInterface + { + return JsonResponse::fromArray(); + } + + public function redirectAction(): ResponseInterface + { + return new RedirectResponse('https://example.com'); + } +} + +?> diff --git a/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/Fixture/skip_extbase_controller_actions.php.inc b/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/Fixture/skip_extbase_controller_actions.php.inc new file mode 100644 index 000000000..de07db26e --- /dev/null +++ b/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/Fixture/skip_extbase_controller_actions.php.inc @@ -0,0 +1,43 @@ +responseFactory->createResponse(200); + throw new PropagateResponseException($response, 200); + } + + public function someOtherAction(): ResponseInterface + { + return new ForwardResponse('another'); + } + + public function somethingAction($range): ResponseInterface + { + $years = array_map(static function ($item): string { + return 'C'.$item.'-'; + }, $range); + + return $this->htmlResponse($years); + } + + public function listAction(): ResponseInterface + { + } + + public function redirectAction(): ResponseInterface + { + return $this->redirect(); + } + + abstract public function listAction($filter = null, string $letter = ''): ResponseInterface; +} +?> diff --git a/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/Source/JsonResponse.php b/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/Source/JsonResponse.php new file mode 100644 index 000000000..463243a8e --- /dev/null +++ b/tests/Rector/v12/v0/ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector/Source/JsonResponse.php @@ -0,0 +1,13 @@ +import(__DIR__ . '/../../../../../../config/config_test.php'); + $rectorConfig->rule(ExtbaseActionsWithRedirectMustReturnResponseInterfaceRector::class); +};