Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Configurable phpDoc tags for FQCN processing #7628

Merged
merged 3 commits into from
Dec 28, 2023
Merged

feat: Configurable phpDoc tags for FQCN processing #7628

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06aad50
Configurable phpDoc tags for FQCN processing
Wirone Dec 27, 2023
1b2ef5d
Do not parse phpDoc if list of allowed tags is empty
Wirone Dec 27, 2023
9416449
Merge branch 'master' into codito/config-option-for-fqcn-in-phpdoc
keradus Dec 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions doc/rules/import/fully_qualified_strict_types.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ Allowed types: ``bool``

Default value: ``false``

``phpdoc_tags``
~~~~~~~~~~~~~~~

Collection of PHPDoc annotation tags where FQCNs should be processed. As of now
only simple tags with ``@tag \F\Q\C\N`` format are supported (no complex types).

Allowed types: ``array``

Default value: ``['param', 'phpstan-param', 'phpstan-property', 'phpstan-property-read', 'phpstan-property-write', 'phpstan-return', 'phpstan-var', 'property', 'property-read', 'property-write', 'psalm-param', 'psalm-property', 'psalm-property-read', 'psalm-property-write', 'psalm-return', 'psalm-var', 'return', 'see', 'throws', 'var']``
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I would personally remove @see which is required to be FQCN in some coding standards and very often is not a FQCN alone, it is very often a method name, which is not a "type" which this fixer is about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but as a second condition fixer matches simple FQCNs with a regex, so URLs or methods won't be processed 🙂.


Examples
--------

Expand Down
36 changes: 35 additions & 1 deletion src/Fixer/Import/FullyQualifiedStrictTypesFixer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,34 @@ protected function createConfigurationDefinition(): FixerConfigurationResolverIn
->setAllowedTypes(['bool'])
->setDefault(false)
->getOption(),
(new FixerOptionBuilder(
'phpdoc_tags',
'Collection of PHPDoc annotation tags where FQCNs should be processed. As of now only simple tags with `@tag \F\Q\C\N` format are supported (no complex types).'
))
->setAllowedTypes(['array'])
->setDefault([
'param',
'phpstan-param',
'phpstan-property',
'phpstan-property-read',
'phpstan-property-write',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would be the case of phpstan-property-read enabled, but phpstan-property-write disabled?

imagine someone actually use the config, phpstan introducing new annotation - they need to manually add the annotation here.
I would rather have a group of annotations like @phpstan or phpstan* for all phpstan-based annotations

(same for others)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are more phpstan-* and psalm-* annotations, but these have different purpose. I only included these related to types, tool-specific variants of regular phpDoc ones. I doubt new annotations will emerge 🙂.

Maybe we can focus on regular phpDoc tags, and automatically add prefixed ones 🤔? Like when someone includes param and automatically phpstan-param and psalm-param are included. It would be more logical to me than wildcards, because you may want to process param but not var (prefixed variants). The other alternative is to ignore tool-specific variants in default config and just let people configure it by themselves, but as far as I remember other fixers handle these too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like, tbh I see only 2 cases myself - want to process annotations (any of them) or not. boolean config - do we have request to go granular and support some, but not all annotations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example @see is something that I think can be processed, and there was a voice it shouldn't be. Also, having this configurable makes it possible to clear large codebases step by step (by enabling one tag after another and providing partial diffs). List of tags was previously hardcoded and there was a request to add new tags - having it configurable allows people to do it in a flexible way, without changing the fixer's code.

'phpstan-return',
'phpstan-var',
'property',
'property-read',
'property-write',
'psalm-param',
'psalm-property',
'psalm-property-read',
'psalm-property-write',
'psalm-return',
'psalm-var',
'return',
'see',
'throws',
'var',
])
->getOption(),
]);
}

Expand Down Expand Up @@ -260,13 +288,19 @@ private function fixFunction(FunctionsAnalyzer $functionsAnalyzer, Tokens $token
*/
private function fixPhpDoc(Tokens $tokens, int $index, array $uses, string $namespaceName): void
{
$allowedTags = $this->configuration['phpdoc_tags'];

if ([] === $allowedTags) {
return;
}

$phpDoc = $tokens[$index];
$phpDocContent = $phpDoc->getContent();
Preg::matchAll('#@([^\s]+)\s+([^\s]+)#', $phpDocContent, $matches);

if ([] !== $matches) {
foreach ($matches[2] as $i => $typeName) {
if (!\in_array($matches[1][$i], ['param', 'return', 'see', 'throws', 'var'], true)) {
if (!\in_array($matches[1][$i], $allowedTags, true)) {
continue;
}

Expand Down
109 changes: 104 additions & 5 deletions tests/Fixer/Import/FullyQualifiedStrictTypesFixerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,7 +1072,7 @@ public function testCodeWithPhpDoc(string $expected, ?string $input = null, arra
}

/**
* @return iterable<string, array{0: string, 1?: string, 2?: array<string, mixed>}>
* @return iterable<string, array{0: string, 1?: null|string, 2?: array<string, mixed>}>
*/
public static function provideCodeWithPhpDocCases(): iterable
{
Expand Down Expand Up @@ -1251,14 +1251,24 @@ final class SomeClass {}',
/**
* @see Baz
* @see Bam
*
* @property SomeClass $foo
* @property-read Buz $buz
* @property-write Baz $baz
* @phpstan-property SomeClass $foo
* @phpstan-property-read Buz $buz
* @phpstan-property-write Baz $baz
* @psalm-property SomeClass $foo
* @psalm-property-read Buz $buz
* @psalm-property-write Baz $baz
*/
interface SomeClass
{
/**
* @param SomeClass $foo
* @param Buz $buz
* @phpstan-param Buz $buz
*
* @return Baz
* @psalm-return Baz
*/
public function doSomething(SomeClass $foo, Buz $buz): Baz;
}',
Expand All @@ -1273,14 +1283,24 @@ public function doSomething(SomeClass $foo, Buz $buz): Baz;
/**
* @see \Foo\Bar\Baz
* @see \Foo\Bar\Bam
*
* @property \Foo\Bar\SomeClass $foo
* @property-read \Foo\Bar\Buz $buz
* @property-write \Foo\Bar\Baz $baz
* @phpstan-property \Foo\Bar\SomeClass $foo
* @phpstan-property-read \Foo\Bar\Buz $buz
* @phpstan-property-write \Foo\Bar\Baz $baz
* @psalm-property \Foo\Bar\SomeClass $foo
* @psalm-property-read \Foo\Bar\Buz $buz
* @psalm-property-write \Foo\Bar\Baz $baz
*/
interface SomeClass
{
/**
* @param \Foo\Bar\SomeClass $foo
* @param \Foo\Bar\Buz $buz
* @phpstan-param \Foo\Bar\Buz $buz
*
* @return \Foo\Bar\Baz
* @psalm-return \Foo\Bar\Baz
*/
public function doSomething(\Foo\Bar\SomeClass $foo, \Foo\Bar\Buz $buz): \Foo\Bar\Baz;
}',
Expand Down Expand Up @@ -1411,6 +1431,85 @@ function foo($dateTime) {}',
function foo($dateTime) {}',
['leading_backslash_in_global_namespace' => true],
];

yield 'Do not touch PHPDoc if configured with empty collection' => [
'<?php

namespace Foo\Bar;

use Foo\Bar\Buz;
use Foo\Bar\Baz;
use Foo\Bar\SomeClass;

/**
* @see \Foo\Bar\Baz
* @see \Foo\Bar\Bam
*
* @property \Foo\Bar\SomeClass $foo
* @property-read \Foo\Bar\Buz $buz
* @property-write \Foo\Bar\Baz $baz
*/
interface SomeClass
{
/**
* @param \Foo\Bar\SomeClass $foo
* @phpstan-param \Foo\Bar\Buz $buz
*
* @psalm-return \Foo\Bar\Baz
*/
public function doSomething($foo, $buz);
}',
null,
['phpdoc_tags' => []],
];

yield 'Process only specified PHPDoc annotation' => [
'<?php

namespace Foo\Bar;

use Foo\Baz\Buzz;

/**
* @see \Foo\Baz\Buzz
*
* @property \Foo\Baz\Buzz $buzz1
* @property-read Buzz $buzz2
*/
interface SomeClass
{
/**
* @param \Foo\Baz\Buzz $a
* @phpstan-param Buzz $b
*
* @psalm-return \Foo\Baz\Buzz
*/
public function doSomething($a, $b): void;
}',
'<?php

namespace Foo\Bar;

use Foo\Baz\Buzz;

/**
* @see \Foo\Baz\Buzz
*
* @property \Foo\Baz\Buzz $buzz1
* @property-read \Foo\Baz\Buzz $buzz2
*/
interface SomeClass
{
/**
* @param \Foo\Baz\Buzz $a
* @phpstan-param \Foo\Baz\Buzz $b
*
* @psalm-return \Foo\Baz\Buzz
*/
public function doSomething($a, $b): void;
}',
['phpdoc_tags' => ['property-read', 'phpstan-param']],
];
}

/**
Expand Down