bug symfony#23844 [Debug] Correctly detect methods not from the same …

…vendor (GuilhemN)

This PR was merged into the 3.4 branch.

Discussion
----------

[Debug] Correctly detect methods not from the same vendor

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

I just realized there were two issues in symfony#23816:

* when a class extended another class from the same vendor itself extending a class from another vendor containing deprecated methods, the deprecation wasn't triggered in case the first class extended a deprecated method from the last class because the parent from the same vendor was used for "the same vendor check".

* As stated in symfony#23816 (comment), ReflectionMethod::$class doesn't contain the right value when using a trait so if you extended a trait containing methods from another vendor you would have always get deprecations. I fixed this by comparing the method file to the filename of the class using the trait.

@nicolas-grekas are you aware of any issue of `ReflectionMethod::getFilename()`?

Commits
-------

08d352a [Debug] Correctly detect methods not from the same vendor

.travis.yml

@@ -20,17 +20,8 @@ env:
 
 matrix:
     include:
-        # Use the newer stack for HHVM as HHVM does not support Precise anymore since a long time and so Precise has an outdated version
-        - php: hhvm-3.18
-          sudo: required
-          dist: trusty
-          group: edge
         - php: 5.5
         - php: 5.6
-        - php: 7.1
-          env: deps=high
-        - php: 7.0
-          env: deps=low
     fast_finish: true
 
 cache:
@@ -91,13 +82,7 @@ before_install:
           echo extension = mongodb.so >> $INI
       fi
 
-      # Matrix lines for intermediate PHP versions are skipped for pull requests
-      if [[ ! $deps && ! $PHP = ${MIN_PHP%.*} && ! $PHP = hhvm* && $TRAVIS_PULL_REQUEST != false ]]; then
-          deps=skip
-          skip=1
-      else
-          COMPONENTS=$(find src/Symfony -mindepth 3 -type f -name phpunit.xml.dist -printf '%h\n')
-      fi
+      COMPONENTS=$(find src/Symfony -mindepth 3 -type f -name phpunit.xml.dist -printf '%h\n')
 
     - |
       # Install sigchild-enabled PHP to test the Process component on the lowest PHP matrix line

src/Symfony/Component/Debug/DebugClassLoader.php

@@ -232,19 +232,28 @@ public function loadClass($class)
                     continue;
                 }
 
+                // Method from a trait
+                if ($method->getFilename() !== $refl->getFileName()) {
+                    continue;
+                }
+
                 if ($isClass && $parent && isset(self::$finalMethods[$parent][$method->name])) {
-                    list($methodShortName, $message) = self::$finalMethods[$parent][$method->name];
-                    @trigger_error(sprintf('The "%s" method is considered final%s. It may change without further notice as of its next major version. You should not extend it from "%s".', $methodShortName, $message, $name), E_USER_DEPRECATED);
+                    list($declaringClass, $message) = self::$finalMethods[$parent][$method->name];
+                    @trigger_error(sprintf('The "%s::%s()" method is considered final%s. It may change without further notice as of its next major version. You should not extend it from "%s".', $declaringClass, $method->name, $message, $name), E_USER_DEPRECATED);
                 }
 
                 foreach ($parentAndTraits as $use) {
-                    if (isset(self::$deprecatedMethods[$use][$method->name]) && strncmp($ns, $use, $len)) {
-                        list($methodShortName, $message) = self::$deprecatedMethods[$use][$method->name];
-                        @trigger_error(sprintf('The "%s" method is deprecated%s. You should not extend it from "%s".', $methodShortName, $message, $name), E_USER_DEPRECATED);
+                    if (isset(self::$deprecatedMethods[$use][$method->name])) {
+                        list($declaringClass, $message) = self::$deprecatedMethods[$use][$method->name];
+                        if (strncmp($ns, $declaringClass, $len)) {
+                            @trigger_error(sprintf('The "%s::%s()" method is deprecated%s. You should not extend it from "%s".', $declaringClass, $method->name, $message, $name), E_USER_DEPRECATED);
+                        }
                     }
-                    if (isset(self::$internalMethods[$use][$method->name]) && strncmp($ns, $use, $len)) {
-                        list($methodShortName, $message) = self::$internalMethods[$use][$method->name];
-                        @trigger_error(sprintf('The "%s" method is considered internal%s. It may change without further notice. You should not use it from "%s".', $methodShortName, $message, $name), E_USER_DEPRECATED);
+                    if (isset(self::$internalMethods[$use][$method->name])) {
+                        list($declaringClass, $message) = self::$internalMethods[$use][$method->name];
+                        if (strncmp($ns, $declaringClass, $len)) {
+                            @trigger_error(sprintf('The "%s::%s()" method is considered internal%s. It may change without further notice. You should not extend it from "%s".', $declaringClass, $method->name, $message, $name), E_USER_DEPRECATED);
+                        }
                     }
                 }
 
@@ -256,7 +265,7 @@ public function loadClass($class)
                 foreach (array('final', 'deprecated', 'internal') as $annotation) {
                     if (false !== strpos($doc, '@'.$annotation) && preg_match('#\n\s+\* @'.$annotation.'(?:( .+?)\.?)?\r?\n\s+\*(?: @|/$)#s', $doc, $notice)) {
                         $message = isset($notice[1]) ? preg_replace('#\s*\r?\n \* +#', ' ', $notice[1]) : '';
-                        self::${$annotation.'Methods'}[$name][$method->name] = array(sprintf('%s::%s()', $name, $method->name), $message);
+                        self::${$annotation.'Methods'}[$name][$method->name] = array($name, $message);
                     }
                 }
             }

src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php

@@ -347,10 +347,10 @@ class_exists('Test\\'.__NAMESPACE__.'\\ExtendsInternals', true);
         restore_error_handler();
 
         $this->assertSame($deprecations, array(
+            'The "Symfony\Component\Debug\Tests\Fixtures\InternalClass" class is considered internal since version 3.4. It may change without further notice. You should not use it from "Test\Symfony\Component\Debug\Tests\ExtendsInternalsParent".',
+            'The "Symfony\Component\Debug\Tests\Fixtures\InternalInterface" interface is considered internal. It may change without further notice. You should not use it from "Test\Symfony\Component\Debug\Tests\ExtendsInternalsParent".',
             'The "Symfony\Component\Debug\Tests\Fixtures\InternalTrait" trait is considered internal. It may change without further notice. You should not use it from "Test\Symfony\Component\Debug\Tests\ExtendsInternals".',
-            'The "Symfony\Component\Debug\Tests\Fixtures\InternalClass" class is considered internal since version 3.4. It may change without further notice. You should not use it from "Test\Symfony\Component\Debug\Tests\ExtendsInternals".',
-            'The "Symfony\Component\Debug\Tests\Fixtures\InternalInterface" interface is considered internal. It may change without further notice. You should not use it from "Test\Symfony\Component\Debug\Tests\ExtendsInternals".',
-            'The "Symfony\Component\Debug\Tests\Fixtures\InternalClass::internalMethod()" method is considered internal since version 3.4. It may change without further notice. You should not use it from "Test\Symfony\Component\Debug\Tests\ExtendsInternals".',
+            'The "Symfony\Component\Debug\Tests\Fixtures\InternalTrait2::internalMethod()" method is considered internal since version 3.4. It may change without further notice. You should not extend it from "Test\Symfony\Component\Debug\Tests\ExtendsInternals".',
         ));
     }
 }
@@ -399,11 +399,13 @@ public function findFile($class)
                 public function deprecatedMethod() { }
             }');
         } elseif ('Test\\'.__NAMESPACE__.'\ExtendsInternals' === $class) {
-            eval('namespace Test\\'.__NAMESPACE__.'; class ExtendsInternals extends \\'.__NAMESPACE__.'\Fixtures\InternalClass implements \\'.__NAMESPACE__.'\Fixtures\InternalInterface {
+            eval('namespace Test\\'.__NAMESPACE__.'; class ExtendsInternals extends ExtendsInternalsParent {
                 use \\'.__NAMESPACE__.'\Fixtures\InternalTrait;
 
                 public function internalMethod() { }
             }');
+        } elseif ('Test\\'.__NAMESPACE__.'\ExtendsInternalsParent' === $class) {
+            eval('namespace Test\\'.__NAMESPACE__.'; class ExtendsInternalsParent extends \\'.__NAMESPACE__.'\Fixtures\InternalClass implements \\'.__NAMESPACE__.'\Fixtures\InternalInterface { }');
         }
     }
 }

src/Symfony/Component/Lock/Tests/Store/MemcachedStoreTest.php

@@ -11,6 +11,8 @@
 
 namespace Symfony\Component\Lock\Tests\Store;
 
+use Psr\Log\AbstractLogger;
+use Symfony\Component\Lock\Key;
 use Symfony\Component\Lock\Store\MemcachedStore;
 
 /**
@@ -49,4 +51,97 @@ public function getStore()
 
         return new MemcachedStore($memcached);
     }
+
+    /**
+     * @dataProvider lotOfData
+     */
+    public function testSaveWithDifferentResources()
+    {
+        $store = $this->getStore();
+
+        $key1 = new Key(uniqid(__METHOD__, true));
+        $key2 = new Key(uniqid(__METHOD__, true));
+
+        $store->save($key1);
+        try {
+            $this->assertTrue($store->exists($key1));
+        } catch (\Exception $e) {
+            var_dump($key1);
+            var_dump($key2);
+            $memcached = new \Memcached();
+            $memcached->addServer(getenv('MEMCACHED_HOST'), 11211);
+            var_dump($memcached->get((string) $key1));
+
+            throw $e;
+        }
+        try {
+            $this->assertFalse($store->exists($key2));
+        } catch (\Exception $e) {
+            var_dump($key1);
+            var_dump($key2);
+            $memcached = new \Memcached();
+            $memcached->addServer(getenv('MEMCACHED_HOST'), 11211);
+            var_dump($memcached->get((string) $key2));
+
+            throw $e;
+        }
+
+        $store->save($key2);
+
+        $this->assertTrue($store->exists($key1));
+        $this->assertTrue($store->exists($key2));
+
+        $store->delete($key1);
+        $this->assertFalse($store->exists($key1));
+        $store->delete($key2);
+        $this->assertFalse($store->exists($key2));
+    }
+
+    /**
+     * @dataProvider lotOfData
+     */
+    public function testSaveWithDifferentKeysOnSameResources()
+    {
+        $store = $this->getStore();
+
+        $resource = uniqid(__METHOD__, true);
+        $key1 = new Key($resource);
+        $key2 = new Key($resource);
+
+        $store->save($key1);
+        $this->assertTrue($store->exists($key1));
+        $this->assertFalse($store->exists($key2));
+
+        try {
+            $store->save($key2);
+            var_dump($key1);
+            var_dump($key2);
+            $memcached = new \Memcached();
+            $memcached->addServer(getenv('MEMCACHED_HOST'), 11211);
+            var_dump($memcached->get($resource));
+            throw new \Exception('The store shouldn\'t save the second key');
+        } catch (LockConflictedException $e) {
+        }
+
+        // The failure of previous attempt should not impact the state of current locks
+        $this->assertTrue($store->exists($key1));
+        $this->assertFalse($store->exists($key2));
+
+        $store->delete($key1);
+        $this->assertFalse($store->exists($key1));
+        $this->assertFalse($store->exists($key2));
+
+        $store->save($key2);
+        $this->assertFalse($store->exists($key1));
+        $this->assertTrue($store->exists($key2));
+
+        $store->delete($key2);
+        $this->assertFalse($store->exists($key1));
+        $this->assertFalse($store->exists($key2));
+    }
+
+    public function lotOfData()
+    {
+        return array_fill(0, 1000, []);
+    }
 }