Repository: phpactor/phpactor (php)

Issue:
Refactor: Extract constant overwrites already existing constant
Hey,
i just noticed that the refactor option `extract constant` overwrites already existing constants with the same name. Here is an example:

```php
class Example
{
	public function __construct()
	{
		$this->foobar("This is an example", 'This is also a test');
	}

	public function foobar(string $param1, string $param2): void
	{
	}
}
```

When i extract the first string i get the following:
```php
class Example
{
    const NEW_CONSTANT = 'This is an example';

	public function __construct()
	{
		$this->foobar(self::NEW_CONSTANT, 'This is also a test');
	}

	public function foobar(string $param1, string $param2): void
	{
	}
}
```

But when i want to extract the second string i get the following:
```php
class Example
{
    const NEW_CONSTANT = 'This is an example';

	public function __construct()
	{
		$this->foobar(self::NEW_CONSTANT, self::NEW_CONSTANT);
	}

	public function foobar(string $param1, string $param2): void
	{
	}
}
```

This is wrong, because the first and the second string are not the same. A quick workaround for this is to rename the first constant before refactoring the second one. But maybe phpactor should choose a new name (`NEW_CONSTANT_2` for example) to avoid data loss or confusion while refactoring.

Thanks so much and greetings
Leo

Pull request:
Do not overwrite constants when extracting
Closes #2166 

Patch:
diff --git a/doc/reference/configuration.rst b/doc/reference/configuration.rst
index 83000a09..91111820 100644
--- a/doc/reference/configuration.rst
+++ b/doc/reference/configuration.rst
@@ -1237,7 +1237,7 @@ Specify which diagnostic providers should be active (default to all)
 If applicable diagnostics should be "outsourced" to a different process
 
 
-**Default**: ``false``
+**Default**: ``true``
 
 
 .. _param_language_server,file_events:
diff --git a/lib/CodeTransform/Adapter/WorseReflection/Refactor/WorseExtractConstant.php b/lib/CodeTransform/Adapter/WorseReflection/Refactor/WorseExtractConstant.php
index 4ac98711..a7a60ae3 100644
--- a/lib/CodeTransform/Adapter/WorseReflection/Refactor/WorseExtractConstant.php
+++ b/lib/CodeTransform/Adapter/WorseReflection/Refactor/WorseExtractConstant.php
@@ -64,7 +64,17 @@ class WorseExtractConstant implements ExtractConstant
         $classType = $symbolInformation->containerType()->expandTypes()->classLike()->firstOrNull();
 
         if (!$classType) {
-            throw new TransformException(sprintf('Node does not belong to a class'));
+            throw new TransformException('Node does not belong to a class');
+        }
+
+        if ($classType->members()->constants()->has($constantName)) {
+            throw new TransformException(
+                sprintf(
+                    'Constant with name %s already exists on class %s',
+                    $constantName,
+                    $classType->name()->short()
+                )
+            );
         }
 
         $builder->namespace($classType->name()->namespace());
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
index e086df37..f086bfde 100644
--- a/phpstan-baseline.neon
+++ b/phpstan-baseline.neon
@@ -830,16 +830,6 @@ parameters:
 			count: 1
 			path: lib/CodeTransform/Tests/Adapter/WorseReflection/Helper/WorseInterestingOffsetFinderTest.php
 
-		-
-			message: "#^Method Phpactor\\\\CodeTransform\\\\Tests\\\\Adapter\\\\WorseReflection\\\\Refactor\\\\WorseExtractConstantTest\\:\\:provideExtractMethod\\(\\) has no return type specified\\.$#"
-			count: 1
-			path: lib/CodeTransform/Tests/Adapter/WorseReflection/Refactor/WorseExtractConstantTest.php
-
-		-
-			message: "#^Method Phpactor\\\\CodeTransform\\\\Tests\\\\Adapter\\\\WorseReflection\\\\Refactor\\\\WorseExtractConstantTest\\:\\:testExtractConstant\\(\\) has parameter \\$name with no type specified\\.$#"
-			count: 1
-			path: lib/CodeTransform/Tests/Adapter/WorseReflection/Refactor/WorseExtractConstantTest.php
-
 		-
 			message: "#^Method Phpactor\\\\CodeTransform\\\\Tests\\\\Adapter\\\\WorseReflection\\\\Refactor\\\\WorseGenerateAccessorTest\\:\\:provideExtractAccessor\\(\\) has no return type specified\\.$#"
 			count: 1


Test patch:
diff --git a/lib/CodeTransform/Tests/Adapter/WorseReflection/Refactor/WorseExtractConstantTest.php b/lib/CodeTransform/Tests/Adapter/WorseReflection/Refactor/WorseExtractConstantTest.php
index 9fba8bf8..607aa7b6 100644
--- a/lib/CodeTransform/Tests/Adapter/WorseReflection/Refactor/WorseExtractConstantTest.php
+++ b/lib/CodeTransform/Tests/Adapter/WorseReflection/Refactor/WorseExtractConstantTest.php
@@ -2,17 +2,19 @@
 
 namespace Phpactor\CodeTransform\Tests\Adapter\WorseReflection\Refactor;
 
+use Generator;
 use Phpactor\CodeTransform\Tests\Adapter\WorseReflection\WorseTestCase;
 use Phpactor\CodeTransform\Adapter\WorseReflection\Refactor\WorseExtractConstant;
 use Phpactor\CodeTransform\Domain\SourceCode;
 use Phpactor\CodeTransform\Domain\Exception\TransformException;
+use Phpactor\TestUtils\ExtractOffset;
 
 class WorseExtractConstantTest extends WorseTestCase
 {
     /**
      * @dataProvider provideExtractMethod
      */
-    public function testExtractConstant(string $test, $name): void
+    public function testExtractConstant(string $test, string $name): void
     {
         [$source, $expected, $offset] = $this->sourceExpectedAndOffset(__DIR__ . '/fixtures/' . $test);
 
@@ -26,39 +28,18 @@ class WorseExtractConstantTest extends WorseTestCase
 
         $this->assertEquals(trim($expected), trim($transformed));
     }
-
-    public function provideExtractMethod()
+    /**
+     * @return Generator<string,array<int,string>>
+     */
+    public function provideExtractMethod(): Generator
     {
-        return [
-            'string' => [
-                'extractConstant1.test',
-                'HELLO_WORLD'
-            ],
-            'numeric' => [
-                'extractConstant2.test',
-                'HELLO_WORLD'
-            ],
-            'array_key' => [
-                'extractConstant3.test',
-                'HELLO_WORLD'
-            ],
-            'namespaced' => [
-                'extractConstant4.test',
-                'HELLO_WORLD'
-            ],
-            'replace all' => [
-                'extractConstant5.test',
-                'HELLO_WORLD'
-            ],
-            'replace all numeric' => [
-                'extractConstant6.test',
-                'HOUR'
-            ],
-            'replace heredoc' => [
-                'extractConstant7.test',
-                'HELLO_WORLD'
-            ],
-        ];
+        yield 'string' => [ 'extractConstant1.test', 'HELLO_WORLD' ];
+        yield 'numeric' => [ 'extractConstant2.test', 'HELLO_WORLD' ];
+        yield 'array_key' => [ 'extractConstant3.test', 'HELLO_WORLD' ];
+        yield 'namespaced' => [ 'extractConstant4.test', 'HELLO_WORLD' ];
+        yield 'replace all' => [ 'extractConstant5.test', 'HELLO_WORLD' ];
+        yield 'replace all numeric' => [ 'extractConstant6.test', 'HOUR' ];
+        yield 'replace heredoc' => [ 'extractConstant7.test', 'HELLO_WORLD' ];
     }
 
     public function testNoClass(): void
@@ -72,6 +53,28 @@ class WorseExtractConstantTest extends WorseTestCase
         ;
 
         $extractConstant = new WorseExtractConstant($this->reflectorForWorkspace($code), $this->updater());
-        $transformed = $extractConstant->extractConstant(SourceCode::fromString($code), 8, 'asd');
+        $extractConstant->extractConstant(SourceCode::fromString($code), 8, 'asd');
+    }
+
+    public function testNoOverwritingOfExistingConstants(): void
+    {
+        $this->expectException(TransformException::class);
+        $this->expectExceptionMessage('Constant with name TEXT already exists on class Test');
+
+        $code = <<<'EOT'
+            <?php
+            class Test {
+                const TEXT = 'Cool constant';
+
+                public function doSomething(): void
+                {
+                    echo 'SomeT<>ext';
+                }
+            }
+            EOT;
+
+        [$source, $offset] = ExtractOffset::fromSource($code);
+        $extractConstant = new WorseExtractConstant($this->reflectorForWorkspace($source), $this->updater());
+        $extractConstant->extractConstant(SourceCode::fromString($source), $offset, 'TEXT');
     }
 }