Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/DependencyInjection/ParameterBag/ContainerBagInterface.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| * ACCESS_GRANTED, ACCESS_DENIED, or ACCESS_ABSTAIN. | ||
| * | ||
| * @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 onphpstan/phpstan-symfony#291, this may be changed toarray<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 anyarray a generic (so requiring to specify at least one type). I think it's OK to use Psalm & PHPstorm's definition ofarray = 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 wherearray is used and could be replaced bystring[] or others more precise types. So currently,array just mean "We didn't precise, but maybe don't accept everything" and not "Everyarray<array-key, mixed> is allowed".
PHPStan strategy force to pass on everyarray to write instead, sometimesstring[], sometimearray<string, mixed> and sometimesarray<mixed>. The advantage ofarray<mixed> (ormixed[] if preferred) is the fact than we do know that the function really accept everything, it's supposed to be checked when changingarray intoarray<mixed> (orstring[] or ...).
I may be wrong but I don't feel like changingarray intoarray<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 usingarray<mixed>.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
stof commentedJul 21, 2022
@wouterj you need to update the |
Uh oh!
There was an error while loading.Please reload this page.
wouterj commentedJul 23, 2022
This one is ready to merge now imho |
Nyholm left a comment
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)
Uh oh!
There was an error while loading.Please reload this page.
wouterj commentedJul 23, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/DependencyInjection/ParameterBag/ContainerBagInterface.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
…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/aReplaces#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
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedAug 2, 2022
Thank you@wouterj. |
nicolas-grekas commentedAug 2, 2022
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/AWhen 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#L40When 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 | Replacessymfony#40783| License | MIT| Doc PR |symfony/symfony-docs#17064Todo---* [x] Review the Psalm check of this PR* [x] Test the changes with DebugClassLoader in a real appDescription---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 bysymfony/symfony-docs#17064On top of this, to get this PR approved quicker, I've left out all stubs from the Form (based on the discussions insymfony#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 keyfa7703b [ErrorHandler] Do not patch return types that are not a valid PHP type38ab6de 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 | MITI think this was a typo in#47016.Commits-------e1a5beb [Messenger] fix `Envelope::all()` conditional return docblock
Uh oh!
There was an error while loading.Please reload this page.
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 bysymfony/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 ofPHPStan's
Envelopestub (due to complexity of the PHPdoc).