[PhpUnitBridge] Use serialized trace when using serialized deprecation

symfony · l-vo · May 6, 2019 · May 8, 2019 · May 15, 2019 · May 15, 2019
commit e827025f6a7c96b003ee1fed3460d6cd04563f5e
diff --git a/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php b/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php
@@ -108,7 +108,19 @@ public static function collectDeprecations($outputFile)
                 return $ErrorHandler::handleError($type, $msg, $file, $line, $context);
             }
 
-            $deprecations[] = [error_reporting(), $msg, $file];
+            $trace = debug_backtrace();
+            $filesStack = [];
+            foreach ($trace as $line) {
+                if (\in_array($line['function'], ['require', 'require_once', 'include', 'include_once'], true)) {
+                    continue;
+                }
+
+                if (isset($line['file'])) {
+                    $filesStack[] = $line['file'];
+                }
+            }
+
+            $deprecations[] = [error_reporting(), $msg, $file, $filesStack];
         });
 
         register_shutdown_function(function () use ($outputFile, &$deprecations) {

diff --git a/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/Deprecation.php b/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/Deprecation.php
@@ -46,6 +46,8 @@ class Deprecation
     /** @var string[] absolute paths to vendor directories */
     private static $vendors;
 
+    private $originalFilesStack;
+
     /**
      * @param string $message
      * @param string $file
@@ -66,6 +68,7 @@ public function __construct($message, array $trace, $file)
                 $this->message = $parsedMsg['deprecation'];
                 $this->originClass = $parsedMsg['class'];
                 $this->originMethod = $parsedMsg['method'];
+                $this->originalFilesStack = $parsedMsg['files_stack'];
                 // If the deprecation has been triggered via
                 // \Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListenerTrait::endTest()
                 // then we need to use the serialized information to determine
@@ -159,6 +162,24 @@ public function isLegacy($utilPrefix)
             || \in_array('legacy', $test::getGroups($class, $method), true);
     }
 
+    private function getOriginalFilesStack(): array
+    {
+        if (null === $this->originalFilesStack) {
+            $this->originalFilesStack = [];
+            foreach ($this->trace as $line) {
+                if (\in_array($line['function'], ['require', 'require_once', 'include', 'include_once'], true)) {
+                    continue;
+                }
+                if (!isset($line['file'])) {
+                    continue;
+                }
+                $this->originalFilesStack[] = $line['file'];
+            }
+        }
+
+        return $this->originalFilesStack;
+    }
+
     /**
      * Tells whether both the calling package and the called package are vendor
      * packages.
@@ -168,14 +189,8 @@ public function isLegacy($utilPrefix)
     public function isIndirect()
     {
         $erroringFile = $erroringPackage = null;
-        foreach ($this->trace as $line) {
-            if (\in_array($line['function'], ['require', 'require_once', 'include', 'include_once'], true)) {
-                continue;
-            }
-            if (!isset($line['file'])) {
-                continue;
-            }
-            $file = $line['file'];
+
+        foreach ($this->getOriginalFilesStack() as $file) {
             if ('-' === $file || 'Standard input code' === $file || !realpath($file)) {
                 continue;
             }

diff --git a/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php b/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php
@@ -32,10 +32,10 @@ class SymfonyTestsListenerTrait
     private static $globallyEnabled = false;
     private $state = -1;
     private $skippedFile = false;
-    private $wasSkipped = array();
-    private $isSkipped = array();
-    private $expectedDeprecations = array();
-    private $gatheredDeprecations = array();
+    private $wasSkipped = [];
+    private $isSkipped = [];
+    private $expectedDeprecations = [];
+    private $gatheredDeprecations = [];
     private $previousErrorHandler;
     private $testsWithWarnings;
     private $reportUselessTests;
@@ -45,7 +45,7 @@ class SymfonyTestsListenerTrait
     /**
      * @param array $mockedNamespaces List of namespaces, indexed by mocked features (time-sensitive or dns-sensitive)
      */
-    public function __construct(array $mockedNamespaces = array())
+    public function __construct(array $mockedNamespaces = [])
     {
         if (class_exists('PHPUnit_Util_Blacklist')) {
             \PHPUnit_Util_Blacklist::$blacklistedClassNames['\Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListenerTrait'] = 2;
@@ -57,7 +57,7 @@ public function __construct(array $mockedNamespaces = array())
 
         foreach ($mockedNamespaces as $type => $namespaces) {
             if (!\is_array($namespaces)) {
-                $namespaces = array($namespaces);
+                $namespaces = [$namespaces];
             }
             if ('time-sensitive' === $type) {
                 foreach ($namespaces as $ns) {
@@ -114,7 +114,7 @@ public function startTestSuite($suite)
             $Test = 'PHPUnit\Util\Test';
         }
         $suiteName = $suite->getName();
-        $this->testsWithWarnings = array();
+        $this->testsWithWarnings = [];
 
         foreach ($suite->tests() as $test) {
             if (!($test instanceof \PHPUnit\Framework\TestCase || $test instanceof TestCase)) {
@@ -145,11 +145,11 @@ public function startTestSuite($suite)
 
                     if (!$this->wasSkipped = require $this->skippedFile) {
                         echo "All tests already ran successfully.\n";
-                        $suite->setTests(array());
+                        $suite->setTests([]);
                     }
                 }
             }
-            $testSuites = array($suite);
+            $testSuites = [$suite];
             for ($i = 0; isset($testSuites[$i]); ++$i) {
                 foreach ($testSuites[$i]->tests() as $test) {
                     if ($test instanceof \PHPUnit_Framework_TestSuite || $test instanceof TestSuite) {
@@ -168,7 +168,7 @@ public function startTestSuite($suite)
                 }
             }
         } elseif (2 === $this->state) {
-            $skipped = array();
+            $skipped = [];
             foreach ($suite->tests() as $test) {
                 if (!($test instanceof \PHPUnit\Framework\TestCase || $test instanceof TestCase)
                     || isset($this->wasSkipped[$suiteName]['*'])
@@ -240,7 +240,7 @@ public function startTest($test)
                 $test->getTestResultObject()->beStrictAboutTestsThatDoNotTestAnything(false);
 
                 $this->expectedDeprecations = $annotations['method']['expectedDeprecation'];
-                $this->previousErrorHandler = set_error_handler(array($this, 'handleError'));
+                $this->previousErrorHandler = set_error_handler([$this, 'handleError']);
             }
         }
     }
@@ -281,8 +281,14 @@ public function endTest($test, $time)
             $deprecations = file_get_contents($this->runsInSeparateProcess);
             unlink($this->runsInSeparateProcess);
             putenv('SYMFONY_DEPRECATIONS_SERIALIZE');
-            foreach ($deprecations ? unserialize($deprecations) : array() as $deprecation) {
-                $error = serialize(array('deprecation' => $deprecation[1], 'class' => $className, 'method' => $test->getName(false), 'triggering_file' => isset($deprecation[2]) ? $deprecation[2] : null));
+            foreach ($deprecations ? unserialize($deprecations) : [] as $deprecation) {
+                $error = serialize([
+                    'deprecation' => $deprecation[1],
+                    'class' => $className,
+                    'method' => $test->getName(false),
+                    'triggering_file' => isset($deprecation[2]) ? $deprecation[2] : null,
+                    'files_stack' => $deprecation[3],
+                ]);
                 if ($deprecation[0]) {
                     @trigger_error($error, E_USER_DEPRECATED);
                 } else {
@@ -293,13 +299,13 @@ public function endTest($test, $time)
         }
 
         if ($this->expectedDeprecations) {
-            if (!\in_array($test->getStatus(), array($BaseTestRunner::STATUS_SKIPPED, $BaseTestRunner::STATUS_INCOMPLETE), true)) {
+            if (!\in_array($test->getStatus(), [$BaseTestRunner::STATUS_SKIPPED, $BaseTestRunner::STATUS_INCOMPLETE], true)) {
                 $test->addToAssertionCount(\count($this->expectedDeprecations));
             }
 
             restore_error_handler();
 
-            if (!$errored && !\in_array($test->getStatus(), array($BaseTestRunner::STATUS_SKIPPED, $BaseTestRunner::STATUS_INCOMPLETE, $BaseTestRunner::STATUS_FAILURE, $BaseTestRunner::STATUS_ERROR), true)) {
+            if (!$errored && !\in_array($test->getStatus(), [$BaseTestRunner::STATUS_SKIPPED, $BaseTestRunner::STATUS_INCOMPLETE, $BaseTestRunner::STATUS_FAILURE, $BaseTestRunner::STATUS_ERROR], true)) {
                 try {
                     $prefix = "@expectedDeprecation:\n";
                     $test->assertStringMatchesFormat($prefix.'%A  '.implode("\n%A  ", $this->expectedDeprecations)."\n%A", $prefix.'  '.implode("\n  ", $this->gatheredDeprecations)."\n");
@@ -310,20 +316,20 @@ public function endTest($test, $time)
                 }
             }
 
-            $this->expectedDeprecations = $this->gatheredDeprecations = array();
+            $this->expectedDeprecations = $this->gatheredDeprecations = [];
             $this->previousErrorHandler = null;
         }
         if (!$this->runsInSeparateProcess && -2 < $this->state && ($test instanceof \PHPUnit\Framework\TestCase || $test instanceof TestCase)) {
             if (\in_array('time-sensitive', $groups, true)) {
                 ClockMock::withClockMock(false);
             }
             if (\in_array('dns-sensitive', $groups, true)) {
-                DnsMock::withMockedHosts(array());
+                DnsMock::withMockedHosts([]);
             }
         }
     }
 
-    public function handleError($type, $msg, $file, $line, $context = array())
+    public function handleError($type, $msg, $file, $line, $context = [])
     {
         if (E_USER_DEPRECATED !== $type && E_DEPRECATED !== $type) {
             $h = $this->previousErrorHandler;

diff --git a/src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/DeprecationTest.php b/src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/DeprecationTest.php
@@ -11,12 +11,31 @@
 
 namespace Symfony\Bridge\PhpUnit\Tests\DeprecationErrorHandler;
 
+use Composer\Autoload\ClassLoader;
 use PHPUnit\Framework\TestCase;
 use Symfony\Bridge\PhpUnit\DeprecationErrorHandler\Deprecation;
 use Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListenerForV5;
 
 class DeprecationTest extends TestCase
 {
+    public static function setUpBeforeClass(): void
+    {
+        $vendorDir = self::getVendorDir();
+
+        mkdir($vendorDir.'/myfakevendor/myfakepackage1', 0777, true);
+        mkdir($vendorDir.'/myfakevendor/myfakepackage2');
+        touch($vendorDir.'/myfakevendor/myfakepackage1/MyFakeFile1.php');
+        touch($vendorDir.'/myfakevendor/myfakepackage1/MyFakeFile2.php');
+        touch($vendorDir.'/myfakevendor/myfakepackage2/MyFakeFile.php');
+    }
+
+    private static function getVendorDir(): string
+    {
+        $reflection = new \ReflectionClass(ClassLoader::class);
+
+        return \dirname($reflection->getFileName(), 2);
+    }
+
     public function testItCanDetermineTheClassWhereTheDeprecationHappened()
     {
         $deprecation = new Deprecation('💩', $this->debugBacktrace(), __FILE__);
@@ -101,6 +120,58 @@ public function testIsSelf(bool $expectedIsSelf, string $message, string $traceC
         $this->assertEquals($expectedIsSelf, $deprecation->isSelf());
     }
 
+    public function providerIsIndirectUsesRightTrace(): array
+    {
+        $vendorDir = self::getVendorDir();
+
+        return [
+            'no_file_in_stack' => [false, '', [['function' => 'myfunc1'], ['function' => 'myfunc2']]],
+            'files_in_stack_from_various_packages' => [
+                true,
+                '',
+                [
+                    ['function' => 'myfunc1', 'file' => $vendorDir.'/myfakevendor/myfakepackage1/MyFakeFile1.php'],
+                    ['function' => 'myfunc2', 'file' => $vendorDir.'/myfakevendor/myfakepackage2/MyFakeFile.php'],
+                ],
+            ],
+            'serialized_stack_files_from_same_package' => [
+                false,
+                serialize([
+                    'deprecation' => 'My deprecation message',
+                    'class' => 'MyClass',
+                    'method' => 'myMethod',
+                    'files_stack' => [
+                        $vendorDir.'/myfakevendor/myfakepackage1/MyFakeFile1.php',
+                        $vendorDir.'/myfakevendor/myfakepackage1/MyFakeFile2.php',
+                    ],
+                ]),
+                [['function' => 'myfunc1'], ['class' => SymfonyTestsListenerForV5::class, 'method' => 'mymethod']],
+            ],
+            'serialized_stack_files_from_various_packages' => [
+                true,
+                serialize([
+                    'deprecation' => 'My deprecation message',
+                    'class' => 'MyClass',
+                    'method' => 'myMethod',
+                    'files_stack' => [
+                        $vendorDir.'/myfakevendor/myfakepackage1/MyFakeFile1.php',
+                        $vendorDir.'/myfakevendor/myfakepackage2/MyFakeFile.php',
+                    ],
+                ]),
+                [['function' => 'myfunc1'], ['class' => SymfonyTestsListenerForV5::class, 'method' => 'mymethod']],
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider providerIsIndirectUsesRightTrace
+     */
+    public function testIsIndirectUsesRightTrace(bool $expectedIsIndirect, string $message, array $trace): void
+    {
+        $deprecation = new Deprecation($message, $trace, '');
+        $this->assertEquals($expectedIsIndirect, $deprecation->isIndirect());
+    }
+
     /**
      * This method is here to simulate the extra level from the piece of code
      * triggering an error to the error handler.
@@ -109,4 +180,22 @@ public function debugBacktrace(): array
     {
         return debug_backtrace();
     }
+
+    private static function removeDir($dir): void
+    {
+        $files = glob($dir.'/*');
+        foreach ($files as $file) {
+            if (is_file($file)) {
+                unlink($file);
+            } else {
+                self::removeDir($file);
+            }
+        }
+        rmdir($dir);
+    }
+
+    public static function tearDownAfterClass(): void
+    {
+        self::removeDir(self::getVendorDir().'/myfakevendor');
+    }
 }