[BUGFIX] Only validate method params if needed 70/56970/7
authorAlexander Schnitzler <git@alexanderschnitzler.de>
Tue, 15 May 2018 13:46:29 +0000 (15:46 +0200)
committerBenni Mack <benni@typo3.org>
Tue, 22 May 2018 14:28:39 +0000 (16:28 +0200)
Controller action arguments have been validated on
creation, which caused superfluous CPU cycles if the
action controller later detected, that an argument
should not have been validated at all due to an
@Extbase\IgnoreValidation annotation.

To fix this, arguments get an empty result on creation.
When setting the argument value, only the validation
results of the property mapping are merged with the
argument result.

ActionController::initializeActionMethodValidators does
only create validator instances for method arguments
that need to be validated, thus a whole bunch of checks
disappears in callActionMethod().

Releases: master
Resolves: #85012
Change-Id: Iaecf36718477a9216f8d36a993a137eb7b677227
Reviewed-on: https://review.typo3.org/56970
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Wouter Wolters <typo3@wouterwolters.nl>
Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Reviewed-by: Benni Mack <benni@typo3.org>
Tested-by: Benni Mack <benni@typo3.org>
typo3/sysext/core/Documentation/Changelog/master/Deprecation-85012-OnlyValidateMethodParamsIfNeeded.rst [new file with mode: 0644]
typo3/sysext/extbase/Classes/Mvc/Controller/ActionController.php
typo3/sysext/extbase/Classes/Mvc/Controller/Argument.php
typo3/sysext/extbase/Classes/Mvc/Controller/Arguments.php
typo3/sysext/extbase/Tests/Unit/Mvc/Controller/ArgumentTest.php
typo3/sysext/install/Configuration/ExtensionScanner/Php/MethodCallMatcher.php

diff --git a/typo3/sysext/core/Documentation/Changelog/master/Deprecation-85012-OnlyValidateMethodParamsIfNeeded.rst b/typo3/sysext/core/Documentation/Changelog/master/Deprecation-85012-OnlyValidateMethodParamsIfNeeded.rst
new file mode 100644 (file)
index 0000000..6d09a51
--- /dev/null
@@ -0,0 +1,38 @@
+.. include:: ../../Includes.txt
+
+===========================================================
+Deprecation: #85012 - Only validate method params if needed
+===========================================================
+
+See :issue:`85012`
+
+Description
+===========
+
+The following public methods have been marked as deprecated:
+
+* :php:`TYPO3\CMS\Extbase\Mvc\Controller\Argument::getValidationResults()`
+* :php:`TYPO3\CMS\Extbase\Mvc\Controller\Arguments::getValidationResults()`
+
+Impact
+======
+
+Calling the method triggers a deprecation log entry.
+
+
+Affected Installations
+======================
+
+Extensions that call that method. The extension scanner will find usages.
+
+
+Migration
+=========
+
+Use the following methods instead:
+
+* :php:`TYPO3\CMS\Extbase\Mvc\Controller\Argument::validate()`
+* :php:`TYPO3\CMS\Extbase\Mvc\Controller\Arguments::validate()`
+
+
+.. index:: PHP-API, FullyScanned, ext:extbase
index 4ace0e0..0b7aa14 100644 (file)
@@ -257,8 +257,16 @@ class ActionController extends AbstractController
 
         $classSchema = $this->reflectionService->getClassSchema(static::class);
 
+        $ignoreValidationAnnotations = array_unique(array_flip(
+            $classSchema->getMethod($this->actionMethodName)['tags']['ignorevalidation'] ?? []
+        ));
+
         /** @var \TYPO3\CMS\Extbase\Mvc\Controller\Argument $argument */
         foreach ($this->arguments as $argument) {
+            if (isset($ignoreValidationAnnotations[$argument->getName()])) {
+                continue;
+            }
+
             $validator = $this->objectManager->get(ConjunctionValidator::class);
             $validatorDefinitions = $classSchema->getMethod($this->actionMethodName)['params'][$argument->getName()]['validators'] ?? [];
 
@@ -313,33 +321,12 @@ class ActionController extends AbstractController
         foreach ($this->arguments as $argument) {
             $preparedArguments[] = $argument->getValue();
         }
-        $validationResult = $this->arguments->getValidationResults();
+        $validationResult = $this->arguments->validate();
         if (!$validationResult->hasErrors()) {
             $this->emitBeforeCallActionMethodSignal($preparedArguments);
-            $actionResult = call_user_func_array([$this, $this->actionMethodName], $preparedArguments);
+            $actionResult = $this->{$this->actionMethodName}(...$preparedArguments);
         } else {
-            $methodTagsValues = $this->reflectionService->getMethodTagsValues(static::class, $this->actionMethodName);
-            $ignoreValidationAnnotations = $methodTagsValues['ignorevalidation'] ?? [];
-
-            // if there exist errors which are not ignored with @TYPO3\CMS\Extbase\Annotation\IgnoreValidation => call error method
-            // else => call action method
-            $shouldCallActionMethod = true;
-            foreach ($validationResult->getSubResults() as $argumentName => $subValidationResult) {
-                if (!$subValidationResult->hasErrors()) {
-                    continue;
-                }
-                if (in_array($argumentName, $ignoreValidationAnnotations, true)) {
-                    continue;
-                }
-                $shouldCallActionMethod = false;
-                break;
-            }
-            if ($shouldCallActionMethod) {
-                $this->emitBeforeCallActionMethodSignal($preparedArguments);
-                $actionResult = call_user_func_array([$this, $this->actionMethodName], $preparedArguments);
-            } else {
-                $actionResult = call_user_func([$this, $this->errorMethodName]);
-            }
+            $actionResult = $this->{$this->errorMethodName}();
         }
 
         if ($actionResult === null && $this->view instanceof ViewInterface) {
@@ -610,7 +597,7 @@ class ActionController extends AbstractController
         if ($referringRequest !== null) {
             $originalRequest = clone $this->request;
             $this->request->setOriginalRequest($originalRequest);
-            $this->request->setOriginalRequestMappingResults($this->arguments->getValidationResults());
+            $this->request->setOriginalRequestMappingResults($this->arguments->validate());
             $this->forward(
                 $referringRequest->getControllerActionName(),
                 $referringRequest->getControllerName(),
index e13efb9..798ae6f 100644 (file)
@@ -14,6 +14,7 @@ namespace TYPO3\CMS\Extbase\Mvc\Controller;
  * The TYPO3 project - inspiring people to share!
  */
 
+use TYPO3\CMS\Extbase\Error\Result;
 use TYPO3\CMS\Extbase\Property\Exception\TargetNotFoundException;
 use TYPO3\CMS\Extbase\Utility\TypeHandlingUtility;
 
@@ -88,7 +89,12 @@ class Argument
      *
      * @var \TYPO3\CMS\Extbase\Error\Result
      */
-    protected $validationResults = null;
+    protected $validationResults;
+
+    /**
+     * @var bool
+     */
+    private $hasBeenValidated = false;
 
     /**
      * @param \TYPO3\CMS\Extbase\Property\PropertyMapper $propertyMapper
@@ -124,6 +130,8 @@ class Argument
         }
         $this->name = $name;
         $this->dataType = TypeHandlingUtility::normalizeType($dataType);
+
+        $this->validationResults = new Result();
     }
 
     /**
@@ -274,12 +282,7 @@ class Argument
                 throw $e;
             }
         }
-        $this->validationResults = $this->propertyMapper->getMessages();
-        if ($this->validator !== null) {
-            // @todo Validation API has also changed!!!
-            $validationMessages = $this->validator->validate($this->value);
-            $this->validationResults->merge($validationMessages);
-        }
+        $this->validationResults->merge($this->propertyMapper->getMessages());
         return $this;
     }
 
@@ -312,18 +315,23 @@ class Argument
      * @return bool TRUE if the argument is valid, FALSE otherwise
      * @api
      */
-    public function isValid()
+    public function isValid(): bool
     {
-        return !$this->validationResults->hasErrors();
+        return !$this->validate()->hasErrors();
     }
 
     /**
      * @return \TYPO3\CMS\Extbase\Error\Result Validation errors which have occurred.
-     * @api
+     * @deprecated
      */
     public function getValidationResults()
     {
-        return $this->validationResults;
+        trigger_error(
+            'Method ' . __METHOD__ . ' is deprecated and will be removed in TYPO3 v10.0.',
+            E_USER_DEPRECATED
+        );
+
+        return $this->validate();
     }
 
     /**
@@ -336,4 +344,23 @@ class Argument
     {
         return (string)$this->value;
     }
+
+    /**
+     * @return \TYPO3\CMS\Extbase\Error\Result
+     * @api
+     */
+    public function validate(): \TYPO3\CMS\Extbase\Error\Result
+    {
+        if ($this->hasBeenValidated) {
+            return $this->validationResults;
+        }
+
+        if ($this->validator !== null) {
+            $validationMessages = $this->validator->validate($this->value);
+            $this->validationResults->merge($validationMessages);
+        }
+
+        $this->hasBeenValidated = true;
+        return $this->validationResults;
+    }
 }
index c70dc45..e2936fe 100644 (file)
@@ -275,13 +275,27 @@ class Arguments extends \ArrayObject
      * Get all property mapping / validation errors
      *
      * @return \TYPO3\CMS\Extbase\Error\Result
+     * @deprecated
      */
     public function getValidationResults()
     {
+        trigger_error(
+            'Method ' . __METHOD__ . ' is deprecated and will be removed in TYPO3 v10.0.',
+            E_USER_DEPRECATED
+        );
+
+        return $this->validate();
+    }
+
+    /**
+     * @return \TYPO3\CMS\Extbase\Error\Result
+     */
+    public function validate(): \TYPO3\CMS\Extbase\Error\Result
+    {
         $results = new \TYPO3\CMS\Extbase\Error\Result();
         /** @var Argument $argument */
         foreach ($this as $argument) {
-            $argumentValidationResults = $argument->getValidationResults();
+            $argumentValidationResults = $argument->validate();
             if ($argumentValidationResults === null) {
                 continue;
             }
index 8ce7092..165b479 100644 (file)
@@ -231,7 +231,7 @@ class ArgumentTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
         $this->simpleValueArgument->setValidator($mockValidator);
         $this->setupPropertyMapperAndSetValue();
         $this->assertFalse($this->simpleValueArgument->isValid());
-        $this->assertEquals([$error], $this->simpleValueArgument->getValidationResults()->getErrors());
+        $this->assertEquals([$error], $this->simpleValueArgument->validate()->getErrors());
     }
 
     /**
index af8dbf5..1dc067d 100644 (file)
@@ -2249,4 +2249,18 @@ return [
             'Deprecation-85005-DeprecateMethodsAndConstantsInValidatorResolver.rst',
         ],
     ],
+    'TYPO3\CMS\Extbase\Mvc\Controller\Argument->getValidationResults' => [
+        'numberOfMandatoryArguments' => 0,
+        'maximumNumberOfArguments' => 0,
+        'restFiles' => [
+            'Deprecation-85012-OnlyValidateMethodParamsIfNeeded.rst',
+        ],
+    ],
+    'TYPO3\CMS\Extbase\Mvc\Controller\Arguments->getValidationResults' => [
+        'numberOfMandatoryArguments' => 0,
+        'maximumNumberOfArguments' => 0,
+        'restFiles' => [
+            'Deprecation-85012-OnlyValidateMethodParamsIfNeeded.rst',
+        ],
+    ],
 ];