From b142ee6e5dc143f39a22f213bd78f07045a2ecda Mon Sep 17 00:00:00 2001
From: Aydin Hassan <a.hassan@shopware.com>
Date: Thu, 5 Sep 2024 11:47:15 +0200
Subject: [PATCH] Add check for adding param to method

---
 bin/roave-backward-compatibility-check.php    |   5 +
 .../MethodBased/MethodParameterAdded.php      |  47 +++++
 .../MethodBased/MethodParameterAddedTest.php  | 178 ++++++++++++++++++
 3 files changed, 230 insertions(+)
 create mode 100644 src/DetectChanges/BCBreak/MethodBased/MethodParameterAdded.php
 create mode 100644 test/unit/DetectChanges/BCBreak/MethodBased/MethodParameterAddedTest.php

diff --git a/bin/roave-backward-compatibility-check.php b/bin/roave-backward-compatibility-check.php
index 055b568e..473825ae 100644
--- a/bin/roave-backward-compatibility-check.php
+++ b/bin/roave-backward-compatibility-check.php
@@ -145,6 +145,7 @@ static function (string $installationPath) use ($composerIo): Installer {
                                             new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodConcretenessChanged()),
                                             new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodScopeChanged()),
                                             new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodVisibilityReduced()),
+                                            new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodParameterAdded()),
                                             new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodFunctionDefinitionChanged(
                                                 new FunctionBased\MultipleChecksOnAFunction(
                                                     new FunctionBased\SkipFunctionBasedErrors(new FunctionBased\FunctionBecameInternal()),
@@ -167,6 +168,7 @@ static function (string $installationPath) use ($composerIo): Installer {
                                             new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodConcretenessChanged()),
                                             new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodScopeChanged()),
                                             new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodVisibilityReduced()),
+                                            new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodParameterAdded()),
                                             new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodFunctionDefinitionChanged(
                                                 new FunctionBased\MultipleChecksOnAFunction(
                                                     new FunctionBased\SkipFunctionBasedErrors(new FunctionBased\FunctionBecameInternal()),
@@ -214,6 +216,7 @@ static function (string $installationPath) use ($composerIo): Installer {
                                         new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodBecameFinal()),
                                         new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodConcretenessChanged()),
                                         new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodScopeChanged()),
+                                        new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodParameterAdded()),
                                         new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodVisibilityReduced()),
                                         new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodFunctionDefinitionChanged(
                                             new FunctionBased\MultipleChecksOnAFunction(
@@ -249,6 +252,7 @@ static function (string $installationPath) use ($composerIo): Installer {
                         new ClassBased\SkipClassBasedErrors(new ClassBased\MethodChanged(
                             new MethodBased\MultipleChecksOnAMethod(
                                 new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodScopeChanged()),
+                                new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodParameterAdded()),
                                 new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodFunctionDefinitionChanged(
                                     new FunctionBased\MultipleChecksOnAFunction(
                                         new FunctionBased\SkipFunctionBasedErrors(new FunctionBased\FunctionBecameInternal()),
@@ -293,6 +297,7 @@ static function (string $installationPath) use ($composerIo): Installer {
                                     new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodConcretenessChanged()),
                                     new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodScopeChanged()),
                                     new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodVisibilityReduced()),
+                                    new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodParameterAdded()),
                                     new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodFunctionDefinitionChanged(
                                         new FunctionBased\MultipleChecksOnAFunction(
                                             new FunctionBased\SkipFunctionBasedErrors(new FunctionBased\FunctionBecameInternal()),
diff --git a/src/DetectChanges/BCBreak/MethodBased/MethodParameterAdded.php b/src/DetectChanges/BCBreak/MethodBased/MethodParameterAdded.php
new file mode 100644
index 00000000..432f08a6
--- /dev/null
+++ b/src/DetectChanges/BCBreak/MethodBased/MethodParameterAdded.php
@@ -0,0 +1,47 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Roave\BackwardCompatibility\DetectChanges\BCBreak\MethodBased;
+
+use Psl\Str;
+use Roave\BackwardCompatibility\Change;
+use Roave\BackwardCompatibility\Changes;
+use Roave\BetterReflection\Reflection\ReflectionMethod;
+use Roave\BetterReflection\Reflection\ReflectionParameter;
+
+use function array_diff;
+use function array_map;
+
+/**
+ * Adding a parameter to a method on a non-final class is a BC break.
+ * Any child classes which extend public and protected methods will be incompatible with the new method signature.
+ */
+final class MethodParameterAdded implements MethodBased
+{
+    public function __invoke(ReflectionMethod $fromMethod, ReflectionMethod $toMethod): Changes
+    {
+        if ($fromMethod->getDeclaringClass()->isFinal() || $fromMethod->isPrivate()) {
+            return Changes::empty();
+        }
+
+        $added = array_diff(
+            array_map(static fn (ReflectionParameter $param) => $param->getName(), $toMethod->getParameters()),
+            array_map(static fn (ReflectionParameter $param) => $param->getName(), $fromMethod->getParameters()),
+        );
+
+        return Changes::fromList(
+            ...array_map(
+                static fn (string $paramName): Change => Change::added(
+                    Str\format(
+                        'Parameter %s was added to Method %s() of class %s',
+                        $paramName,
+                        $fromMethod->getName(),
+                        $fromMethod->getDeclaringClass()->getName(),
+                    ),
+                ),
+                $added,
+            ),
+        );
+    }
+}
diff --git a/test/unit/DetectChanges/BCBreak/MethodBased/MethodParameterAddedTest.php b/test/unit/DetectChanges/BCBreak/MethodBased/MethodParameterAddedTest.php
new file mode 100644
index 00000000..81fe85c4
--- /dev/null
+++ b/test/unit/DetectChanges/BCBreak/MethodBased/MethodParameterAddedTest.php
@@ -0,0 +1,178 @@
+<?php
+
+declare(strict_types=1);
+
+namespace RoaveTest\BackwardCompatibility\DetectChanges\BCBreak\MethodBased;
+
+use PHPUnit\Framework\TestCase;
+use Roave\BackwardCompatibility\Change;
+use Roave\BackwardCompatibility\Changes;
+use Roave\BackwardCompatibility\DetectChanges\BCBreak\MethodBased\MethodBased;
+use Roave\BackwardCompatibility\DetectChanges\BCBreak\MethodBased\MethodParameterAdded;
+use Roave\BetterReflection\BetterReflection;
+use Roave\BetterReflection\Reflection\ReflectionClass;
+use Roave\BetterReflection\Reflection\ReflectionMethod;
+use Roave\BetterReflection\Reflector\DefaultReflector;
+use Roave\BetterReflection\SourceLocator\Type\StringSourceLocator;
+
+use function array_combine;
+use function array_keys;
+use function array_map;
+use function assert;
+use function iterator_to_array;
+
+/** @covers \Roave\BackwardCompatibility\DetectChanges\BCBreak\MethodBased\MethodParameterAdded */
+final class MethodParameterAddedTest extends TestCase
+{
+    private MethodBased $methodCheck;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->methodCheck = new MethodParameterAdded();
+    }
+
+    public function testWillSkipCheckingPrivateMethods(): void
+    {
+        $from = $this->createMock(ReflectionMethod::class);
+        $to   = $this->createMock(ReflectionMethod::class);
+
+        $from
+            ->method('isPrivate')
+            ->willReturn(true);
+
+        self::assertEquals(Changes::empty(), ($this->methodCheck)($from, $to));
+    }
+
+    public function testWillSkipCheckingMethodsOnFinalClasses(): void
+    {
+        $from = $this->createMock(ReflectionMethod::class);
+        $to   = $this->createMock(ReflectionMethod::class);
+
+        $from
+            ->method('isPrivate')
+            ->willReturn(false);
+
+        self::assertEquals(Changes::empty(), ($this->methodCheck)($from, $to));
+    }
+
+    public function testWillSkipCheckingPrivateMethodsOnFinalClasses(): void
+    {
+        $from = $this->createMock(ReflectionMethod::class);
+        $to   = $this->createMock(ReflectionMethod::class);
+
+        $from
+            ->method('isPrivate')
+            ->willReturn(true);
+
+        $from
+            ->method('isFinal')
+            ->willReturn(true);
+
+        self::assertEquals(Changes::empty(), ($this->methodCheck)($from, $to));
+    }
+
+    /**
+     * @param string[] $expectedMessages
+     *
+     * @dataProvider methodsToBeTested
+     */
+    public function testDiffs(
+        ReflectionMethod $fromMethod,
+        ReflectionMethod $toMethod,
+        array $expectedMessages,
+    ): void {
+        $changes = (new MethodParameterAdded())($fromMethod, $toMethod);
+
+        self::assertSame(
+            $expectedMessages,
+            array_map(static function (Change $change): string {
+                return $change->__toString();
+            }, iterator_to_array($changes)),
+        );
+    }
+
+    /** @return array<string, array{0: ReflectionMethod, 1: ReflectionMethod, 2: list<string>}> */
+    public function methodsToBeTested(): array
+    {
+        $astLocator = (new BetterReflection())->astLocator();
+
+        $fromLocator = new StringSourceLocator(
+            <<<'PHP'
+<?php
+
+class TheClass {
+    public function addedParameter() {}
+    public function addedTwoParameters() {}
+    public function addedAnotherParameter(int $one) {}
+    public function noParameters() {}
+    public function twoParams(int $one, int $two) {}
+    private function privateMethod() {}
+    public function removedParameter(int $one, array $options = []) {}
+}
+PHP
+            ,
+            $astLocator,
+        );
+
+        $toLocator = new StringSourceLocator(
+            <<<'PHP'
+<?php
+
+class TheClass {
+    public function addedParameter(array $options = []) {}
+    public function addedTwoParameters(int $one, array $options = []) {}
+    public function addedAnotherParameter(int $one, array $options = []) {}
+    public function noParameters() {}
+    public function twoParams(int $one, int $two) {}
+    private function privateMethod(array $options = []) {}
+    public function removedParameter(int $one) {}
+}
+PHP
+            ,
+            $astLocator,
+        );
+
+        $fromClassReflector = new DefaultReflector($fromLocator);
+        $toClassReflector   = new DefaultReflector($toLocator);
+        $fromClass          = $fromClassReflector->reflectClass('TheClass');
+        $toClass            = $toClassReflector->reflectClass('TheClass');
+
+        $methods = [
+            'addedParameter' => ['[BC] ADDED: Parameter options was added to Method addedParameter() of class TheClass'],
+            'addedTwoParameters' => [
+                '[BC] ADDED: Parameter one was added to Method addedTwoParameters() of class TheClass',
+                '[BC] ADDED: Parameter options was added to Method addedTwoParameters() of class TheClass',
+            ],
+            'addedAnotherParameter' => ['[BC] ADDED: Parameter options was added to Method addedAnotherParameter() of class TheClass'],
+            'noParameters' => [],
+            'privateMethod' => [],
+            'removedParameter' => [],
+            'twoParams' => [],
+        ];
+
+        return array_combine(
+            array_keys($methods),
+            array_map(
+                static fn (string $methodName, array $errors): array => [
+                    self::getMethod($fromClass, $methodName),
+                    self::getMethod($toClass, $methodName),
+                    $errors,
+                ],
+                array_keys($methods),
+                $methods,
+            ),
+        );
+    }
+
+    /** @param non-empty-string $name */
+    private static function getMethod(ReflectionClass $class, string $name): ReflectionMethod
+    {
+        $method = $class->getMethod($name);
+
+        assert($method !== null);
+
+        return $method;
+    }
+}