bug: PhpdocSeparationFixer - Make groups handling more flexible (#6668)

doc/list.rst

@@ -2132,9 +2132,13 @@ List of Available Rules
    Configuration options:
 
    - | ``groups``
-     | Sets of annotation types to be grouped together.
+     | Sets of annotation types to be grouped together. Use `*` to match any tag character.
      | Allowed types: ``string[][]``
      | Default value: ``[['deprecated', 'link', 'see', 'since'], ['author', 'copyright', 'license'], ['category', 'package', 'subpackage'], ['property', 'property-read', 'property-write']]``
+   - | ``skip_unlisted_annotations``
+     | Whether to skip annotations that are not listed in any group.
+     | Allowed types: ``bool``
+     | Default value: ``false``
 
 
    Part of rule sets `@PhpCsFixer <./ruleSets/PhpCsFixer.rst>`_ `@Symfony <./ruleSets/Symfony.rst>`_

doc/rules/phpdoc/phpdoc_separation.rst

@@ -12,12 +12,22 @@ Configuration
 ``groups``
 ~~~~~~~~~~
 
-Sets of annotation types to be grouped together.
+Sets of annotation types to be grouped together. Use ``*`` to match any tag
+character.
 
 Allowed types: ``string[][]``
 
 Default value: ``[['deprecated', 'link', 'see', 'since'], ['author', 'copyright', 'license'], ['category', 'package', 'subpackage'], ['property', 'property-read', 'property-write']]``
 
+``skip_unlisted_annotations``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Whether to skip annotations that are not listed in any group.
+
+Allowed types: ``bool``
+
+Default value: ``false``
+
 Examples
 --------
 
@@ -96,6 +106,50 @@ With configuration: ``['groups' => [['author', 'throws', 'custom'], ['return', '
      * @return int  Return the number of changes.
      */
 
+Example #4
+~~~~~~~~~~
+
+With configuration: ``['groups' => [['ORM\\*'], ['Assert\\*']]]``.
+
+.. code-block:: diff
+
+   --- Original
+   +++ New
+    <?php
+    /**
+     * @ORM\Id
+   + * @ORM\GeneratedValue
+     *
+   - * @ORM\GeneratedValue
+     * @Assert\NotNull
+   - *
+     * @Assert\Type("string")
+     */
+
+Example #5
+~~~~~~~~~~
+
+With configuration: ``['skip_unlisted_annotations' => true]``.
+
+.. code-block:: diff
+
+   --- Original
+   +++ New
+    <?php
+    /**
+     * Hello there!
+     *
+     * @author John Doe
+   + *
+     * @custom Test!
+     *
+     * @throws Exception|RuntimeException foo
+     * @param string $foo
+   - *
+     * @param bool   $bar Bar
+     * @return int  Return the number of changes.
+     */
+
 Rule sets
 ---------
 

src/DocBlock/TagComparator.php

@@ -20,6 +20,8 @@ namespace PhpCsFixer\DocBlock;
  *
  * @author Graham Campbell <hello@gjcampbell.co.uk>
  * @author Jakub Kwaśniewski <jakub@zero-85.pl>
+ *
+ * @deprecated
  */
 final class TagComparator
 {
@@ -42,6 +44,8 @@ final class TagComparator
      */
     public static function shouldBeTogether(Tag $first, Tag $second, array $groups = self::DEFAULT_GROUPS): bool
     {
+        @trigger_error('Method '.__METHOD__.' is deprecated and will be removed in version 4.0.', E_USER_DEPRECATED);
+
         $firstName = $first->getName();
         $secondName = $second->getName();
 

src/Fixer/Phpdoc/PhpdocSeparationFixer.php

@@ -17,7 +17,6 @@ namespace PhpCsFixer\Fixer\Phpdoc;
 use PhpCsFixer\AbstractFixer;
 use PhpCsFixer\DocBlock\Annotation;
 use PhpCsFixer\DocBlock\DocBlock;
-use PhpCsFixer\DocBlock\TagComparator;
 use PhpCsFixer\Fixer\ConfigurableFixerInterface;
 use PhpCsFixer\FixerConfiguration\FixerConfigurationResolver;
 use PhpCsFixer\FixerConfiguration\FixerConfigurationResolverInterface;
@@ -25,6 +24,7 @@ use PhpCsFixer\FixerConfiguration\FixerOptionBuilder;
 use PhpCsFixer\FixerDefinition\CodeSample;
 use PhpCsFixer\FixerDefinition\FixerDefinition;
 use PhpCsFixer\FixerDefinition\FixerDefinitionInterface;
+use PhpCsFixer\Preg;
 use PhpCsFixer\Tokenizer\Token;
 use PhpCsFixer\Tokenizer\Tokens;
 use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException;
@@ -66,8 +66,33 @@ EOF;
             'Annotations in PHPDoc should be grouped together so that annotations of the same type immediately follow each other. Annotations of a different type are separated by a single blank line.',
             [
                 new CodeSample($code),
-                new CodeSample($code, ['groups' => [...TagComparator::DEFAULT_GROUPS, ['param', 'return']]]),
-                new CodeSample($code, ['groups' => [['author', 'throws', 'custom'], ['return', 'param']]]),
+                new CodeSample($code, ['groups' => [
+                    ['deprecated', 'link', 'see', 'since'],
+                    ['author', 'copyright', 'license'],
+                    ['category', 'package', 'subpackage'],
+                    ['property', 'property-read', 'property-write'],
+                    ['param', 'return'],
+                ]]),
+                new CodeSample($code, ['groups' => [
+                    ['author', 'throws', 'custom'],
+                    ['return', 'param'],
+                ]]),
+                new CodeSample(
+                    <<<'EOF'
+                    <?php
+                    /**
+                     * @ORM\Id
+                     *
+                     * @ORM\GeneratedValue
+                     * @Assert\NotNull
+                     *
+                     * @Assert\Type("string")
+                     */
+
+                    EOF,
+                    ['groups' => [['ORM\*'], ['Assert\*']]],
+                ),
+                new CodeSample($code, ['skip_unlisted_annotations' => true]),
             ],
         );
     }
@@ -149,11 +174,20 @@ EOF;
         };
 
         return new FixerConfigurationResolver([
-            (new FixerOptionBuilder('groups', 'Sets of annotation types to be grouped together.'))
+            (new FixerOptionBuilder('groups', 'Sets of annotation types to be grouped together. Use `*` to match any tag character.'))
                 ->setAllowedTypes(['string[][]'])
-                ->setDefault(TagComparator::DEFAULT_GROUPS)
+                ->setDefault([
+                    ['deprecated', 'link', 'see', 'since'],
+                    ['author', 'copyright', 'license'],
+                    ['category', 'package', 'subpackage'],
+                    ['property', 'property-read', 'property-write'],
+                ])
                 ->setAllowedValues([$allowTagToBelongToOnlyOneGroup])
                 ->getOption(),
+            (new FixerOptionBuilder('skip_unlisted_annotations', 'Whether to skip annotations that are not listed in any group.'))
+                ->setAllowedTypes(['bool'])
+                ->setDefault(false) // @TODO 4.0: set to `true`.
+                ->getOption(),
         ]);
     }
 
@@ -191,9 +225,11 @@ EOF;
                 break;
             }
 
-            if (TagComparator::shouldBeTogether($annotation->getTag(), $next->getTag(), $this->groups)) {
+            $shouldBeTogether = $this->shouldBeTogether($annotation, $next, $this->groups);
+
+            if (true === $shouldBeTogether) {
                 $this->ensureAreTogether($doc, $annotation, $next);
-            } else {
+            } elseif (false === $shouldBeTogether || !$this->configuration['skip_unlisted_annotations']) {
                 $this->ensureAreSeparate($doc, $annotation, $next);
             }
         }
@@ -231,4 +267,62 @@ EOF;
             $doc->getLine($pos)->remove();
         }
     }
+
+    /**
+     * @param list<list<string>> $groups
+     */
+    private function shouldBeTogether(Annotation $first, Annotation $second, array $groups): ?bool
+    {
+        $firstName = $this->tagName($first);
+        $secondName = $this->tagName($second);
+
+        // A tag could not be read.
+        if (null === $firstName || null === $secondName) {
+            return null;
+        }
+
+        if ($firstName === $secondName) {
+            return true;
+        }
+
+        foreach ($groups as $group) {
+            $firstTagIsInGroup = $this->isInGroup($firstName, $group);
+            $secondTagIsInGroup = $this->isInGroup($secondName, $group);
+
+            if ($firstTagIsInGroup) {
+                return $secondTagIsInGroup;
+            }
+
+            if ($secondTagIsInGroup) {
+                return false;
+            }
+        }
+
+        return null;
+    }
+
+    private function tagName(Annotation $annotation): ?string
+    {
+        Preg::match('/@([a-zA-Z0-9_\\\\-]+(?=\s|$|\())/', $annotation->getContent(), $matches);
+
+        return $matches[1] ?? null;
+    }
+
+    /**
+     * @param list<string> $group
+     */
+    private function isInGroup(string $tag, array $group): bool
+    {
+        foreach ($group as $tagInGroup) {
+            $tagInGroup = str_replace('*', '\*', $tagInGroup);
+            $tagInGroup = preg_quote($tagInGroup, '/');
+            $tagInGroup = str_replace('\\\\\*', '.*?', $tagInGroup);
+
+            if (1 === Preg::match("/^{$tagInGroup}$/", $tag)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
 }

tests/AutoReview/ProjectCodeTest.php

@@ -26,7 +26,6 @@ use PhpCsFixer\Tests\Test\AbstractIntegrationTestCase;
 use PhpCsFixer\Tests\TestCase;
 use PhpCsFixer\Tokenizer\Token;
 use PhpCsFixer\Tokenizer\Tokens;
-use PhpCsFixer\Utils;
 use Symfony\Component\Finder\Finder;
 use Symfony\Component\Finder\SplFileInfo;
 
@@ -526,34 +525,6 @@ final class ProjectCodeTest extends TestCase
         static::assertNull($tokens->getNextNonWhitespace($classyEndIndex), sprintf('File "%s" should only contains a single classy.', $file));
     }
 
-    /**
-     * @dataProvider provideSrcClassCases
-     */
-    public function testThereIsNoTriggerErrorUsedDirectly(string $className): void
-    {
-        if (Utils::class === $className) {
-            $this->expectNotToPerformAssertions(); // This is where "trigger_error" should be
-
-            return;
-        }
-
-        $rc = new \ReflectionClass($className);
-        $tokens = Tokens::fromCode(file_get_contents($rc->getFileName()));
-
-        $triggerErrors = array_filter(
-            $tokens->toArray(),
-            static function (Token $token): bool {
-                return $token->equals([T_STRING, 'trigger_error'], false);
-            }
-        );
-
-        static::assertCount(
-            0,
-            $triggerErrors,
-            sprintf('Class "%s" must not use "trigger_error", it shall use "Util::triggerDeprecation" instead.', $className)
-        );
-    }
-
     /**
      * @dataProvider provideSrcClassCases
      */

tests/DocBlock/TagComparatorTest.php

@@ -31,12 +31,16 @@ final class TagComparatorTest extends TestCase
 {
     /**
      * @dataProvider provideComparatorCases
+     *
+     * @group legacy
      */
     public function testComparatorTogether(string $first, string $second, bool $expected): void
     {
         $tag1 = new Tag(new Line('* @'.$first));
         $tag2 = new Tag(new Line('* @'.$second));
 
+        $this->expectDeprecation('%AMethod PhpCsFixer\DocBlock\TagComparator::shouldBeTogether is deprecated and will be removed in version 4.0.');
+
         static::assertSame($expected, TagComparator::shouldBeTogether($tag1, $tag2));
     }
 
@@ -59,12 +63,16 @@ final class TagComparatorTest extends TestCase
      * @dataProvider provideComparatorWithDefinedGroupsCases
      *
      * @param string[][] $groups
+     *
+     * @group legacy
      */
     public function testComparatorTogetherWithDefinedGroups(array $groups, string $first, string $second, bool $expected): void
     {
         $tag1 = new Tag(new Line('* @'.$first));
         $tag2 = new Tag(new Line('* @'.$second));
 
+        $this->expectDeprecation('%AMethod PhpCsFixer\DocBlock\TagComparator::shouldBeTogether is deprecated and will be removed in version 4.0.');
+
         static::assertSame(
             $expected,
             TagComparator::shouldBeTogether($tag1, $tag2, $groups)

tests/Fixer/Phpdoc/PhpdocSeparationFixerTest.php

@@ -15,7 +15,6 @@ declare(strict_types=1);
 namespace PhpCsFixer\Tests\Fixer\Phpdoc;
 
 use PhpCsFixer\ConfigurationException\InvalidFixerConfigurationException;
-use PhpCsFixer\DocBlock\TagComparator;
 use PhpCsFixer\Tests\Test\AbstractFixerTestCase;
 
 /**
@@ -499,6 +498,7 @@ EOF;
      * Something when wrong!
      *
      * @Hello\Test\Foo(asd)
+     *
      * @Method("GET")
      *
      * @param string $expected
@@ -676,7 +676,14 @@ EOF;
 
     public function testLaravelGroups(): void
     {
-        $this->fixer->configure(['groups' => array_merge(TagComparator::DEFAULT_GROUPS, [['param', 'return']])]);
+        $this->fixer->configure(['groups' => [
+            ['param', 'return'],
+            ['throws'],
+            ['deprecated', 'link', 'see', 'since'],
+            ['author', 'copyright', 'license'],
+            ['category', 'package', 'subpackage'],
+            ['property', 'property-read', 'property-write'],
+        ]]);
 
         $expected = <<<'EOF'
 <?php
@@ -822,9 +829,11 @@ EOF;
      *
      * @param array<string, mixed> $config
      */
-    public function testDocCode(array $config, string $expected, string $input): void
+    public function testDocCode(string $expected, ?string $input = null, ?array $config = null): void
     {
-        $this->fixer->configure($config);
+        if (null !== $config) {
+            $this->fixer->configure($config);
+        }
 
         $this->doTest($expected, $input);
     }
@@ -832,7 +841,7 @@ EOF;
     /**
      * @return array<array<null|array<string, mixed>|string>>
      */
-    public static function provideDocCodeCases(): array
+    public static function provideDocCodeCases(): iterable
     {
         $input = <<<'EOF'
 <?php
@@ -850,10 +859,8 @@ EOF;
 
 EOF;
 
-        return [
-            'laravel' => [
-                ['groups' => array_merge(TagComparator::DEFAULT_GROUPS, [['param', 'return']])],
-                <<<'EOF'
+        yield 'laravel' => [
+            <<<'EOF'
 <?php
 /**
  * Hello there!
@@ -870,12 +877,19 @@ EOF;
  */
 
 EOF,
-                $input,
-            ],
+            $input,
+            ['groups' => [
+                ['param', 'return'],
+                ['throws'],
+                ['deprecated', 'link', 'see', 'since'],
+                ['author', 'copyright', 'license'],
+                ['category', 'package', 'subpackage'],
+                ['property', 'property-read', 'property-write'],
+            ]],
+        ];
 
-            'all_tags' => [
-                ['groups' => [['author', 'throws', 'custom'], ['return', 'param']]],
-                <<<'EOF'
+        yield 'all_tags' => [
+            <<<'EOF'
 <?php
 /**
  * Hello there!
@@ -890,12 +904,12 @@ EOF,
  */
 
 EOF,
-                $input,
-            ],
+            $input,
+            ['groups' => [['author', 'throws', 'custom'], ['return', 'param']]],
+        ];
 
-            'default_groups_standard_tags' => [
-                ['groups' => TagComparator::DEFAULT_GROUPS],
-                <<<'EOF'
+        yield 'default_groups_standard_tags' => [
+            <<<'EOF'
 <?php
 /**
  * Hello there!
@@ -913,7 +927,7 @@ EOF,
  */
 
 EOF,
-                <<<'EOF'
+            <<<'EOF'
 <?php
 /**
  * Hello there!
@@ -926,11 +940,10 @@ EOF,
  */
 
 EOF,
-            ],
+        ];
 
-            'default_groups_all_tags' => [
-                ['groups' => TagComparator::DEFAULT_GROUPS],
-                <<<'EOF'
+        yield 'default_groups_all_tags' => [
+            <<<'EOF'
 <?php
 /**
  * Hello there!
@@ -948,7 +961,7 @@ EOF,
  */
 
 EOF,
-                <<<'EOF'
+            <<<'EOF'
 <?php
 /**
  * Hello there!
@@ -961,7 +974,121 @@ EOF,
  */
 
 EOF,
+        ];
+
+        yield 'Separated unlisted tags with default config' => [
+            <<<'EOF'
+<?php
+/**
+ * @not-in-any-group1
+ *
+ * @not-in-any-group2
+ *
+ * @not-in-any-group3
+ */
+
+EOF,
+            <<<'EOF'
+<?php
+/**
+ * @not-in-any-group1
+ * @not-in-any-group2
+ * @not-in-any-group3
+ */
+
+EOF,
+        ];
+
+        yield 'Skip unlisted tags' => [
+            <<<'EOF'
+<?php
+/**
+ * @in-group-1
+ * @in-group-1-too
+ *
+ * @not-in-any-group1
+ *
+ * @not-in-any-group2
+ * @not-in-any-group3
+ */
+
+EOF,
+            <<<'EOF'
+<?php
+/**
+ * @in-group-1
+ *
+ * @in-group-1-too
+ * @not-in-any-group1
+ *
+ * @not-in-any-group2
+ * @not-in-any-group3
+ */
+
+EOF,
+            [
+                'groups' => [['in-group-1', 'in-group-1-too']],
+                'skip_unlisted_annotations' => true,
             ],
         ];
+
+        yield 'Doctrine annotations' => [
+            <<<'EOF'
+<?php
+/**
+ * @ORM\Id
+ * @ORM\Column(type="integer")
+ * @ORM\GeneratedValue
+ */
+
+EOF,
+            <<<'EOF'
+<?php
+/**
+ * @ORM\Id
+ *
+ * @ORM\Column(type="integer")
+ *
+ * @ORM\GeneratedValue
+ */
+
+EOF,
+            ['groups' => [
+                ['ORM\Id', 'ORM\Column', 'ORM\GeneratedValue'],
+            ]],
+        ];
+
+        yield 'With wildcard' => [
+            <<<'EOF'
+<?php
+/**
+ * @ORM\Id
+ * @ORM\Column(type="integer")
+ * @ORM\GeneratedValue
+ *
+ * @Assert\NotNull
+ * @Assert\Type("string")
+ */
+
+EOF,
+            <<<'EOF'
+<?php
+/**
+ * @ORM\Id
+ *
+ * @ORM\Column(type="integer")
+ *
+ * @ORM\GeneratedValue
+ * @Assert\NotNull
+ *
+ * @Assert\Type("string")
+ */
+
+EOF,
+            ['groups' => [
+                ['ORM\*'],
+                ['Assert\*'],
+            ]],
+        ];
     }
 }