minor #33708 [ErrorHandler] don't throw deprecations for return-types…

… by default (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [ErrorHandler] don't throw deprecations for return-types by default | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Fix #33235 | License | MIT | Doc PR | - As discussed a few times already, in 4.4, `DebugClassLoader` shouldn't trigger deprecations when return types are missing. We'll enable them back in 5.1. Commits ------- 2cb419e [ErrorHandler] don't throw deprecations for return-types by default
symfony · Sep 25, 2019 · 745248f · 745248f
2 parents 89d7931 + 2cb419e
commit 745248f
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 19 deletions.
diff --git a/.github/patch-types.php b/.github/patch-types.php
@@ -1,7 +1,8 @@
 <?php
 
 if (false === getenv('SYMFONY_PATCH_TYPE_DECLARATIONS')) {
-    putenv('SYMFONY_PATCH_TYPE_DECLARATIONS=force=1&php71-compat=0');
+    echo "Please define the SYMFONY_PATCH_TYPE_DECLARATIONS env var when running this script.\n";
+    exit(1);
 }
 
 require __DIR__.'/../.phpunit/phpunit-8.3-0/vendor/autoload.php';

diff --git a/.travis.yml b/.travis.yml
@@ -22,6 +22,7 @@ env:
         - MESSENGER_AMQP_DSN=amqp://localhost/%2f/messages
         - MESSENGER_REDIS_DSN=redis://127.0.0.1:7001/messages
         - SYMFONY_PHPUNIT_DISABLE_RESULT_CACHE=1
+        - SYMFONY_PATCH_TYPE_DECLARATIONS=force=0
 
 matrix:
     include:
@@ -298,6 +299,7 @@ install:
                   ln -sd $(realpath src/Symfony/Contracts) vendor/symfony/contracts
                   sed -i 's/"\*\*\/Tests\/"//' composer.json
                   composer install --optimize-autoloader
+                  export SYMFONY_PATCH_TYPE_DECLARATIONS=force=object
                   php .github/patch-types.php
                   php .github/patch-types.php # ensure the script is idempotent
                   PHPUNIT_X="$PHPUNIT_X,legacy"

diff --git a/src/Symfony/Component/ErrorHandler/DebugClassLoader.php b/src/Symfony/Component/ErrorHandler/DebugClassLoader.php
@@ -24,6 +24,19 @@
  * and will throw an exception if a file is found but does
  * not declare the class.
  *
+ * It can also patch classes to turn docblocks into actual return types.
+ * This behavior is controlled by the SYMFONY_PATCH_TYPE_DECLARATIONS env var,
+ * which is a url-encoded array with the follow parameters:
+ *  - force: any value enables deprecation notices - can be any of:
+ *      - "docblock" to patch only docblock annotations
+ *      - "object" to turn union types to the "object" type when possible (not recommended)
+ *      - "1"/"0" to enable/disable patching of return types
+ *  - php: the target version of PHP - e.g. "7.1" doesn't generate "object" types
+ *
+ * Note that patching doesn't care about any coding style so you'd better to run
+ * php-cs-fixer after, with rules "phpdoc_trim_consecutive_blank_line_separation"
+ * and "no_superfluous_phpdoc_tags" enabled typically.
+ *
  * @author Fabien Potencier <fabien@symfony.com>
  * @author Christophe Coevoet <stof@notk.org>
  * @author Nicolas Grekas <p@tchwork.com>
@@ -141,6 +154,7 @@ class DebugClassLoader
     private $isFinder;
     private $loaded = [];
     private $patchTypes;
+
     private static $caseCheck;
     private static $checkedClasses = [];
     private static $final = [];
@@ -160,6 +174,10 @@ public function __construct(callable $classLoader)
         $this->classLoader = $classLoader;
         $this->isFinder = \is_array($classLoader) && method_exists($classLoader[0], 'findFile');
         parse_str(getenv('SYMFONY_PATCH_TYPE_DECLARATIONS') ?: '', $this->patchTypes);
+        $this->patchTypes += [
+            'force' => null,
+            'php' => null,
+        ];
 
         if (!isset(self::$caseCheck)) {
             $file = file_exists(__FILE__) ? __FILE__ : rtrim(realpath('.'), \DIRECTORY_SEPARATOR);
@@ -551,15 +569,14 @@ public function checkAnnotations(\ReflectionClass $refl, string $class): array
                 }
             }
 
-            $forcePatchTypes = $this->patchTypes['force'] ?? null;
+            $forcePatchTypes = $this->patchTypes['force'];
 
-            if ($canAddReturnType = null !== $forcePatchTypes && false === strpos($method->getFileName(), \DIRECTORY_SEPARATOR.'vendor'.\DIRECTORY_SEPARATOR)) {
+            if ($canAddReturnType = $forcePatchTypes && false === strpos($method->getFileName(), \DIRECTORY_SEPARATOR.'vendor'.\DIRECTORY_SEPARATOR)) {
                 if ('void' !== (self::MAGIC_METHODS[$method->name] ?? 'void')) {
                     $this->patchTypes['force'] = $forcePatchTypes ?: 'docblock';
                 }
 
-                $canAddReturnType = ($this->patchTypes['force'] ?? false)
-                    || false !== strpos($refl->getFileName(), \DIRECTORY_SEPARATOR.'Tests'.\DIRECTORY_SEPARATOR)
+                $canAddReturnType = false !== strpos($refl->getFileName(), \DIRECTORY_SEPARATOR.'Tests'.\DIRECTORY_SEPARATOR)
                     || $refl->isFinal()
                     || $method->isFinal()
                     || $method->isPrivate()
@@ -570,20 +587,20 @@ public function checkAnnotations(\ReflectionClass $refl, string $class): array
             }
 
             if (null !== ($returnType = self::$returnTypes[$class][$method->name] ?? self::MAGIC_METHODS[$method->name] ?? null) && !$method->hasReturnType() && !($doc && preg_match('/\n\s+\* @return +(\S+)/', $doc))) {
-                if ('void' === $returnType) {
+                list($normalizedType, $returnType, $declaringClass, $declaringFile) = \is_string($returnType) ? [$returnType, $returnType, '', ''] : $returnType;
+
+                if ('void' === $normalizedType) {
                     $canAddReturnType = false;
                 }
 
-                list($normalizedType, $returnType, $declaringClass, $declaringFile) = \is_string($returnType) ? [$returnType, $returnType, '', ''] : $returnType;
-
-                if ($canAddReturnType && 'docblock' !== ($this->patchTypes['force'] ?? false)) {
+                if ($canAddReturnType && 'docblock' !== $this->patchTypes['force']) {
                     $this->patchMethod($method, $returnType, $declaringFile, $normalizedType);
                 }
 
                 if (strncmp($ns, $declaringClass, $len)) {
-                    if ($canAddReturnType && 'docblock' === ($this->patchTypes['force'] ?? false) && false === strpos($method->getFileName(), \DIRECTORY_SEPARATOR.'vendor'.\DIRECTORY_SEPARATOR)) {
+                    if ($canAddReturnType && 'docblock' === $this->patchTypes['force'] && false === strpos($method->getFileName(), \DIRECTORY_SEPARATOR.'vendor'.\DIRECTORY_SEPARATOR)) {
                         $this->patchMethod($method, $returnType, $declaringFile, $normalizedType);
-                    } elseif ('' !== $declaringClass) {
+                    } elseif ('' !== $declaringClass && null !== $this->patchTypes['force']) {
                         $deprecations[] = sprintf('Method "%s::%s()" will return "%s" as of its next major version. Doing the same in child class "%s" will be required when upgrading.', $declaringClass, $method->name, $normalizedType, $className);
                     }
                 }
@@ -820,7 +837,7 @@ private function setReturnType(string $types, \ReflectionMethod $method, ?string
             } elseif ($n !== $normalizedType || !preg_match('/^\\\\?[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+(?:\\\\[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+)*+$/', $n)) {
                 if ($iterable) {
                     $normalizedType = $returnType = 'iterable';
-                } elseif ($object) {
+                } elseif ($object && 'object' === $this->patchTypes['force']) {
                     $normalizedType = $returnType = 'object';
                 } else {
                     // ignore multi-types return declarations
@@ -947,7 +964,7 @@ private function patchMethod(\ReflectionMethod $method, string $returnType, stri
             }
         }
 
-        if ('docblock' === ($this->patchTypes['force'] ?? null) || ('object' === $normalizedType && ($this->patchTypes['php71-compat'] ?? false))) {
+        if ('docblock' === $this->patchTypes['force'] || ('object' === $normalizedType && '7.1' === $this->patchTypes['php'])) {
             $returnType = implode('|', $returnType);
 
             if ($method->getDocComment()) {
@@ -1015,7 +1032,7 @@ private static function getUseStatements(string $file): array
 
     private function fixReturnStatements(\ReflectionMethod $method, string $returnType)
     {
-        if (($this->patchTypes['php71-compat'] ?? false) && 'object' === ltrim($returnType, '?') && 'docblock' !== ($this->patchTypes['force'] ?? null)) {
+        if ('7.1' === $this->patchTypes['php'] && 'object' === ltrim($returnType, '?') && 'docblock' !== $this->patchTypes['force']) {
             return;
         }
 
@@ -1026,7 +1043,7 @@ private function fixReturnStatements(\ReflectionMethod $method, string $returnTy
         $fixedCode = $code = file($file);
         $i = (self::$fileOffsets[$file] ?? 0) + $method->getStartLine();
 
-        if ('?' !== $returnType && 'docblock' !== ($this->patchTypes['force'] ?? null)) {
+        if ('?' !== $returnType && 'docblock' !== $this->patchTypes['force']) {
             $fixedCode[$i - 1] = preg_replace('/\)(;?\n)/', "): $returnType\\1", $code[$i - 1]);
         }
 

diff --git a/src/Symfony/Component/ErrorHandler/Tests/DebugClassLoaderTest.php b/src/Symfony/Component/ErrorHandler/Tests/DebugClassLoaderTest.php
@@ -16,16 +16,15 @@
 
 class DebugClassLoaderTest extends TestCase
 {
-    /**
-     * @var int Error reporting level before running tests
-     */
+    private $patchTypes;
     private $errorReporting;
-
     private $loader;
 
     protected function setUp(): void
     {
+        $this->patchTypes = getenv('SYMFONY_PATCH_TYPE_DECLARATIONS');
         $this->errorReporting = error_reporting(E_ALL);
+        putenv('SYMFONY_PATCH_TYPE_DECLARATIONS=force=0');
         $this->loader = [new DebugClassLoader([new ClassLoader(), 'loadClass']), 'loadClass'];
         spl_autoload_register($this->loader, true, true);
     }
@@ -34,6 +33,7 @@ protected function tearDown(): void
     {
         spl_autoload_unregister($this->loader);
         error_reporting($this->errorReporting);
+        putenv('SYMFONY_PATCH_TYPE_DECLARATIONS'.(false !== $this->patchTypes ? '='.$this->patchTypes : ''));
     }
 
     /**