Deprecate using a non-static method as a data provider

Author: sebastianbergmann

ChangeLog-10.0.md

@@ -35,6 +35,8 @@ All notable changes of the PHPUnit 10.0 release series are documented in this fi
 * [#4599](https://github.com/sebastianbergmann/phpunit/issues/4599): Unify cache configuration
 * [#4603](https://github.com/sebastianbergmann/phpunit/issues/4603): Use "property" instead of "attribute" for configuring the backup of static fields
 * [#4656](https://github.com/sebastianbergmann/phpunit/issues/4656): Prevent doubling of `__destruct()`
+* Using a non-static method as a data provider is now deprecated 
+* Declaring a data provider method to require an argument is now deprecated
 * PHPUnit no longer invokes a static method named `suite` on a class that is declared in a file that is passed as an argument to the CLI test runner
 * PHPUnit no longer promotes variables that are global in the bootstrap script's scope to global variables in the test runner's scope (use `$GLOBALS['variable'] = 'value'` instead of `$variable = 'value'` in your bootstrap script)
 * `PHPUnit\Framework\TestCase::$backupGlobals` can no longer be used to enable or disable the backup/restore of global and super-global variables for a test case class

src/Event/Value/Test/TestDox.php

@@ -38,7 +38,22 @@ public static function fromTestCase(TestCase $testCase): self
         );
     }
 
-    public function __construct(string $prettifiedClassName, string $prettifiedMethodName, string $prettifiedAndColorizedMethodName)
+    /**
+     * @psalm-param class-string $className
+     * @psalm-param non-empty-string $methodName
+     */
+    public static function fromClassNameAndMethodName(string $className, string $methodName): self
+    {
+        $prettifier = new NamePrettifier;
+
+        return new self(
+            $prettifier->prettifyTestClassName($className),
+            $prettifier->prettifyTestMethodName($methodName),
+            $prettifier->prettifyTestMethodName($methodName),
+        );
+    }
+
+    private function __construct(string $prettifiedClassName, string $prettifiedMethodName, string $prettifiedAndColorizedMethodName)
     {
         $this->prettifiedClassName              = $prettifiedClassName;
         $this->prettifiedMethodName             = $prettifiedMethodName;

src/Event/Value/Test/TestMethod.php

@@ -9,20 +9,17 @@
  */
 namespace PHPUnit\Event\Code;
 
-use function class_exists;
+use function assert;
 use function is_int;
 use function is_numeric;
-use function method_exists;
 use function sprintf;
 use PHPUnit\Event\TestData\DataFromDataProvider;
 use PHPUnit\Event\TestData\MoreThanOneDataSetFromDataProviderException;
 use PHPUnit\Event\TestData\NoDataSetFromDataProviderException;
 use PHPUnit\Event\TestData\TestDataCollection;
 use PHPUnit\Framework\TestCase;
 use PHPUnit\Metadata\MetadataCollection;
-use PHPUnit\Metadata\Parser\Registry as MetadataRegistry;
-use ReflectionException;
-use ReflectionMethod;
+use PHPUnit\Util\Reflection;
 use SebastianBergmann\Exporter\Exporter;
 
 /**
@@ -36,6 +33,10 @@ final class TestMethod extends Test
      * @psalm-var class-string
      */
     private readonly string $className;
+
+    /**
+     * @psalm-var non-empty-string
+     */
     private readonly string $methodName;
     private readonly int $line;
     private readonly TestDox $testDox;
@@ -47,21 +48,26 @@ final class TestMethod extends Test
      */
     public static function fromTestCase(TestCase $testCase): self
     {
-        $location = self::sourceLocationFor($testCase::class, $testCase->name());
+        $methodName = $testCase->name();
+
+        assert(!empty($methodName));
+
+        $location = Reflection::sourceLocationFor($testCase::class, $methodName);
 
         return new self(
             $testCase::class,
-            $testCase->name(),
+            $methodName,
             $location['file'],
             $location['line'],
             TestDox::fromTestCase($testCase),
-            self::metadataFor($testCase::class, $testCase->name()),
+            MetadataCollection::for($testCase::class, $methodName),
             self::dataFor($testCase),
         );
     }
 
     /**
      * @psalm-param class-string $className
+     * @psalm-param non-empty-string $methodName
      */
     public function __construct(string $className, string $methodName, string $file, int $line, TestDox $testDox, MetadataCollection $metadata, TestDataCollection $testData)
     {
@@ -83,6 +89,9 @@ public function className(): string
         return $this->className;
     }
 
+    /**
+     * @psalm-return non-empty-string
+     */
     public function methodName(): string
     {
         return $this->methodName;
@@ -186,40 +195,4 @@ private static function dataFor(TestCase $testCase): TestDataCollection
 
         return TestDataCollection::fromArray($testData);
     }
-
-    private static function metadataFor(string $className, string $methodName): MetadataCollection
-    {
-        if (class_exists($className)) {
-            if (method_exists($className, $methodName)) {
-                return MetadataRegistry::parser()->forClassAndMethod($className, $methodName);
-            }
-
-            return MetadataRegistry::parser()->forClass($className);
-        }
-
-        return MetadataCollection::fromArray([]);
-    }
-
-    /**
-     * @psalm-param class-string $className
-     *
-     * @psalm-return array{file: string, line: int}
-     */
-    private static function sourceLocationFor(string $className, string $methodName): array
-    {
-        try {
-            $reflector = new ReflectionMethod($className, $methodName);
-
-            $file = $reflector->getFileName();
-            $line = $reflector->getStartLine();
-        } catch (ReflectionException) {
-            $file = 'unknown';
-            $line = 0;
-        }
-
-        return [
-            'file' => $file,
-            'line' => $line,
-        ];
-    }
 }

src/Framework/TestBuilder.php

@@ -27,6 +27,8 @@
 final class TestBuilder
 {
     /**
+     * @psalm-param non-empty-string $methodName
+     *
      * @throws InvalidDataProviderException
      */
     public function build(ReflectionClass $theClass, string $methodName): Test

src/Framework/TestSuite.php

@@ -552,6 +552,8 @@ protected function addTestMethod(ReflectionClass $class, ReflectionMethod $metho
         $className  = $class->getName();
         $methodName = $method->getName();
 
+        assert(!empty($methodName));
+
         try {
             $test = (new TestBuilder)->build($class, $methodName);
         } catch (InvalidDataProviderException $e) {
@@ -563,10 +565,9 @@ protected function addTestMethod(ReflectionClass $class, ReflectionMethod $metho
                     $methodName,
                     $class->getFileName(),
                     $method->getStartLine(),
-                    new TestDox(
-                        $prettifier->prettifyTestClassName($className),
-                        $prettifier->prettifyTestMethodName($methodName),
-                        $prettifier->prettifyTestMethodName($methodName),
+                    TestDox::fromClassNameAndMethodName(
+                        $className,
+                        $methodName
                     ),
                     MetadataCollection::fromArray([]),
                     Event\TestData\TestDataCollection::fromArray([])

src/Metadata/Api/DataProvider.php

@@ -26,11 +26,16 @@
 use function strlen;
 use function substr;
 use function trim;
+use PHPUnit\Event;
+use PHPUnit\Event\Code\TestMethod;
+use PHPUnit\Event\TestData\MoreThanOneDataSetFromDataProviderException;
+use PHPUnit\Event\TestData\TestDataCollection;
 use PHPUnit\Framework\InvalidDataProviderException;
 use PHPUnit\Metadata\DataProvider as DataProviderMetadata;
 use PHPUnit\Metadata\MetadataCollection;
 use PHPUnit\Metadata\Parser\Registry;
 use PHPUnit\Metadata\TestWith;
+use PHPUnit\Util\Reflection;
 use ReflectionClass;
 use ReflectionMethod;
 use Throwable;
@@ -43,6 +48,7 @@ final class DataProvider
 {
     /**
      * @psalm-param class-string $className
+     * @psalm-param non-empty-string $methodName
      *
      * @throws InvalidDataProviderException
      */
@@ -56,7 +62,7 @@ public function providedData(string $className, string $methodName): ?array
         }
 
         if ($dataProvider->isNotEmpty()) {
-            $data = $this->dataProvidedByMethods($dataProvider);
+            $data = $this->dataProvidedByMethods($className, $methodName, $dataProvider);
         } else {
             $data = $this->dataProvidedByMetadata($testWith);
         }
@@ -82,9 +88,12 @@ public function providedData(string $className, string $methodName): ?array
     }
 
     /**
+     * @psalm-param class-string $className
+     * @psalm-param non-empty-string $methodName
+     *
      * @throws InvalidDataProviderException
      */
-    private function dataProvidedByMethods(MetadataCollection $dataProvider): array
+    private function dataProvidedByMethods(string $className, string $methodName, MetadataCollection $dataProvider): array
     {
         $result = [];
 
@@ -94,16 +103,39 @@ private function dataProvidedByMethods(MetadataCollection $dataProvider): array
             try {
                 $class  = new ReflectionClass($_dataProvider->className());
                 $method = $class->getMethod($_dataProvider->methodName());
+                $object = null;
+
+                if (!$method->isStatic()) {
+                    Event\Facade::emitter()->testTriggeredPhpunitDeprecation(
+                        $this->valueObjectForTestMethodWithoutTestData(
+                            $className,
+                            $methodName,
+                        ),
+                        sprintf(
+                            'Data Provider method %s::%s() is not static',
+                            $_dataProvider->className(),
+                            $_dataProvider->methodName()
+                        )
+                    );
 
-                if ($method->isStatic()) {
-                    $object = null;
-                } else {
                     $object = $class->newInstanceWithoutConstructor();
                 }
 
                 if ($method->getNumberOfParameters() === 0) {
                     $data = $method->invoke($object);
                 } else {
+                    Event\Facade::emitter()->testTriggeredPhpunitDeprecation(
+                        $this->valueObjectForTestMethodWithoutTestData(
+                            $className,
+                            $methodName,
+                        ),
+                        sprintf(
+                            'Data Provider method %s::%s() expects an argument',
+                            $_dataProvider->className(),
+                            $_dataProvider->methodName()
+                        )
+                    );
+
                     $data = $method->invoke($object, $_dataProvider->methodName());
                 }
             } catch (Throwable $e) {
@@ -207,4 +239,31 @@ private function dataProvidedByTestWithAnnotation(string $className, string $met
 
         return $data;
     }
+
+    /**
+     * @psalm-param class-string $className
+     * @psalm-param non-empty-string $methodName
+     *
+     * @throws MoreThanOneDataSetFromDataProviderException
+     */
+    private function valueObjectForTestMethodWithoutTestData(string $className, string $methodName): TestMethod
+    {
+        $location = Reflection::sourceLocationFor($className, $methodName);
+
+        return new TestMethod(
+            $className,
+            $methodName,
+            $location['file'],
+            $location['line'],
+            Event\Code\TestDox::fromClassNameAndMethodName(
+                $className,
+                $methodName
+            ),
+            MetadataCollection::for(
+                $className,
+                $methodName
+            ),
+            TestDataCollection::fromArray([])
+        );
+    }
 }

src/Metadata/MetadataCollection.php

@@ -11,9 +11,12 @@
 
 use function array_filter;
 use function array_merge;
+use function class_exists;
 use function count;
+use function method_exists;
 use Countable;
 use IteratorAggregate;
+use PHPUnit\Metadata\Parser\Registry as MetadataRegistry;
 
 /**
  * @internal This class is not covered by the backward compatibility promise for PHPUnit
@@ -27,6 +30,23 @@ final class MetadataCollection implements Countable, IteratorAggregate
      */
     private readonly array $metadata;
 
+    /**
+     * @psalm-param class-string $className
+     * @psalm-param non-empty-string $methodName
+     */
+    public static function for(string $className, string $methodName): self
+    {
+        if (class_exists($className)) {
+            if (method_exists($className, $methodName)) {
+                return MetadataRegistry::parser()->forClassAndMethod($className, $methodName);
+            }
+
+            return MetadataRegistry::parser()->forClass($className);
+        }
+
+        return self::fromArray([]);
+    }
+
     /**
      * @psalm-param list<Metadata> $metadata
      */

src/Util/Reflection.php

@@ -12,13 +12,38 @@
 use PHPUnit\Framework\Assert;
 use PHPUnit\Framework\TestCase;
 use ReflectionClass;
+use ReflectionException;
 use ReflectionMethod;
 
 /**
  * @internal This class is not covered by the backward compatibility promise for PHPUnit
  */
 final class Reflection
 {
+    /**
+     * @psalm-param class-string $className
+     * @psalm-param non-empty-string $methodName
+     *
+     * @psalm-return array{file: string, line: int}
+     */
+    public static function sourceLocationFor(string $className, string $methodName): array
+    {
+        try {
+            $reflector = new ReflectionMethod($className, $methodName);
+
+            $file = $reflector->getFileName();
+            $line = $reflector->getStartLine();
+        } catch (ReflectionException) {
+            $file = 'unknown';
+            $line = 0;
+        }
+
+        return [
+            'file' => $file,
+            'line' => $line,
+        ];
+    }
+
     /**
      * @psalm-return list<ReflectionMethod>
      */