-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Improve some PHPdocs based on existing Symfony stubs in PHPstan and Psalm #47016
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.
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
Conversation
67167c1
to
b753827
Compare
src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/ParameterBag/ContainerBagInterface.php
Outdated
Show resolved
Hide resolved
@@ -33,7 +33,7 @@ interface VoterInterface | |||
* @param mixed $subject The subject to secure | |||
* @param array $attributes An array of attributes associated with the method being invoked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on phpstan/phpstan-symfony#291, this may be changed to array<mixed>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I don't like PHPstan's requirement to make any array
a generic (so requiring to specify at least one type). I think it's OK to use Psalm & PHPstorm's definition of array = array<array-key, mixed>
in Symfony's code base and keep changes like this for a PHPstan stub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple places in the Symfony code where array
is used and could be replaced by string[]
or others more precise types. So currently, array
just mean "We didn't precise, but maybe don't accept everything" and not "Every array<array-key, mixed>
is allowed".
PHPStan strategy force to pass on every array
to write instead, sometimes string[]
, sometime array<string, mixed>
and sometimes array<mixed>
. The advantage of array<mixed>
(or mixed[]
if preferred) is the fact than we do know that the function really accept everything, it's supposed to be checked when changing array
into array<mixed>
(or string[]
or ...).
I may be wrong but I don't feel like changing array
into array<mixed>
on some places would require a lot of extra-maintenance to the symfony/symfony project when it would require to duplicate a lot of classes/interface on phpstan/phpstan-symfony just to solve this. So it would be a nice gesture to Symfony+PHPStan users to consider using array<mixed>
.
b753827
to
b6808dc
Compare
@wouterj you need to update the |
b6808dc
to
a51431d
Compare
a51431d
to
8ccd900
Compare
8ccd900
to
d3ba396
Compare
This one is ready to merge now imho |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About this line: https://github.com/symfony/symfony/pull/47016/files#diff-94e123982c1528593508077df90d9228847942a3b8dc077fd93560b09e5b196bR167
$args[\is_string($k) ? $this->resolveValue($k, $resolving) : $k] = $this->resolveValue($v, $resolving);
Depending on $resolving, this may access the array with a non scalar key. Should we modify the code and throw an exception or should we specify $resolving harder?
(I’m on mobile, sorry for the poor syntax)
This is a good catch by Psalm that is easy to produce in a real Symfony app: parameters:
some_list: [1, 2, 3]
broken_parameter:
'%some_list%': foo Note that |
src/Symfony/Component/DependencyInjection/Extension/ExtensionInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/ParameterBag/ContainerBagInterface.php
Outdated
Show resolved
Hide resolved
958ae00
to
1db9fb4
Compare
1db9fb4
to
f5a802a
Compare
…wouterj) This PR was merged into the 6.2 branch. Discussion ---------- [Config] Add conditional types to conditional builders | Q | A | ------------- | --- | Branch? | 6.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Fix #46546 | License | MIT | Doc PR | n/a Replaces #46581 , submitted to 6.2 instead. This matches with the changes proposed in #47016 and the config builder contained quite a big refactor for comments in 6.x so this avoids nasty conflicts. Original description: > One of the main benefits of using the new PHP config builders in application code is the ability to use PHP community tooling to autocomplete and validate the configuration. In this light, I think it makes sense to add the correct PHPdocs to make Psalm and PHPstan understand the config builders. > > On top of that, as these config classes are live generated, this is not something that can be easily stubbed in the special PHPstan and Psalm Symfony plugins. Commits ------- ce1087e [Config] Add conditional types to conditional builders
Thank you @wouterj. |
And thank you @mdeboer also! |
…Stan (ogizanagi) This PR was merged into the 6.2 branch. Discussion ---------- [Console] Fix `OutputInterface` options int-mask for PHPStan | Q | A | ------------- | --- | Branch? | 6.2 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | N/A <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT | Doc PR | N/A When upgrading an application to 6.2, I encountered this issue with PHPStan: ```shell ------ ------------------------------------------------------------------------------------------------------- Line Pilot/Runner/Output/MultiplexingOutput.php ------ ------------------------------------------------------------------------------------------------------- 79 Default value of the parameter #3 $options (0) of method App\Pilot\Runner\Output\MultiplexingOutput::write() is incompatible with type 1|2|4|16|32|64|128|256. ------ ------------------------------------------------------------------------------------------------------- ``` The `MultiplexingOutput` implements the `OutputInterface` and defined `0` as the default value for `$options`: ```php public function write(string|iterable $messages, bool $newline = false, int $options = 0): void ``` as defined by the interface: https://github.com/symfony/symfony/blob/69f46f231b16be1bce1e2ecfa7f9a1fb192cda22/src/Symfony/Component/Console/Output/OutputInterface.php#L40 When trying to use `self::OUTPUT_NORMAL | self::VERBOSITY_NORMAL` as default value: ```shell ------ ------------------------------------------------------------------------------------------------------- Line Pilot/Runner/Output/MultiplexingOutput.php ------ ------------------------------------------------------------------------------------------------------- 79 Default value of the parameter #3 $options (33) of method App\Pilot\Runner\Output\MultiplexingOutput::write() is incompatible with type 1|2|4|16|32|64|128|256. ------ ------------------------------------------------------------------------------------------------------- ``` Or simply using `Output::write()` with specific options: ```shell ------ ------------------------------------------------------------------------------------------------------- Line Pilot/Runner/Output/MultiplexingOutput.php ------ ------------------------------------------------------------------------------------------------------- 81 Parameter #3 $options of method Symfony\Component\Console\Output\Output::write() expects 1|2|4|16|32|64|128|256, 33 given. ------ ------------------------------------------------------------------------------------------------------- ``` Using PHPStan's [`int-mask-of`](https://phpstan.org/writing-php-code/phpdoc-types#integer-masks) is the solution for this, and allows by nature the `0` value. It was even used [at some point](#47016 (comment)), but reverted to use `self::VERBOSITY_*|self::OUTPUT_*`. However, it does not behave as expected for masks. So, either we use `int-mask-of`, or we revert to `int`? Commits ------- 304a17a [Console] Fix OutputInterface options int-mask for PhpStan
…ubs in PHPstan and Psalm (mdeboer, wouterj) This PR was merged into the 6.2 branch. Discussion ---------- Improve some PHPdocs based on existing Symfony stubs in PHPstan and Psalm | Q | A | ------------- | --- | Branch? | 6.2 | Bug fix? | no | New feature? | no | Deprecations? | no (strictly spoken yes, as I guess DebugClassLoader will trigger deprecations for some now?) | Tickets | Replaces symfony#40783 | License | MIT | Doc PR | symfony/symfony-docs#17064 Todo --- * [x] Review the Psalm check of this PR * [x] Test the changes with DebugClassLoader in a real app Description --- This PR adds some more information to the PHPdoc of Symfony classes. By moving the information from the current stubs of static analyzers into Symfony itself: (1) we can improve the experience for all users, (2) reduce false-positives from our own Psalm check and (3) make sure they are in sync with changes done on these classes. <s>To avoid a PR that is too big to review and maintain, the scope of this PR is deliberately kept narrow: * Only PHPdoc from either [Psalm's Symfony stubs](https://github.com/psalm/psalm-plugin-symfony/tree/master/src/Stubs) or [PHPStan's Symfony stubs](https://github.com/phpstan/phpstan-symfony/tree/1.2.x/stubs) is added * The PHPdoc MUST NOT break PHPstorm * The PHPdoc MUST be supported by both PHPstan and Psalm (those are the only two static analyzers that currently ship an official Symfony plugin afaik) * The PHPdoc SHOULD NOT duplicate anything that can already be deduced from either PHP's type declarations or already existing PHPdoc. * The PHPdoc MUST document the contract and NOT be added to fit a 95% use-case or improve auto-completion.</s> EDIT: Replaced the guidelines by symfony/symfony-docs#17064 On top of this, to get this PR approved quicker, I've left out all stubs from the Form (based on the discussions in symfony#40783) and most of [PHPStan's `Envelope` stub](https://github.com/psalm/psalm-plugin-symfony/blob/master/src/Stubs/common/Component/Messenger/Envelope.stubphp) (due to complexity of the PHPdoc). Commits ------- f5a802a [DependencyInjection] Add nice exception when using non-scalar parameter as array key fa7703b [ErrorHandler] Do not patch return types that are not a valid PHP type 38ab6de Improve some PHPdocs based on existing Symfony stubs in PHPstan and Psalm
…block (kbond) This PR was merged into the 6.4 branch. Discussion ---------- [Messenger] fix `Envelope::all()` conditional return docblock | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | n/a | License | MIT I think this was a typo in #47016. Commits ------- e1a5beb [Messenger] fix `Envelope::all()` conditional return docblock
Todo
Description
This PR adds some more information to the PHPdoc of Symfony classes. By moving the information from the current stubs of static analyzers into Symfony itself: (1) we can improve the experience for all users, (2) reduce false-positives from our own Psalm check and (3) make sure they are in sync with changes done on these classes.
To avoid a PR that is too big to review and maintain, the scope of this PR is deliberately kept narrow:The PHPdoc MUST document the contract and NOT be added to fit a 95% use-case or improve auto-completion.EDIT: Replaced the guidelines by symfony/symfony-docs#17064
On top of this, to get this PR approved quicker, I've left out all stubs from the Form (based on the discussions in #40783) and most of PHPStan's
Envelope
stub (due to complexity of the PHPdoc).