Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
nicolas-grekas merged 3 commits intosymfony:6.2fromwouterj:staticanalysis-stubs
Aug 2, 2022

Conversation

@wouterj
Copy link
Member

@wouterjwouterj commentedJul 21, 2022
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?no
Deprecations?no (strictly spoken yes, as I guess DebugClassLoader will trigger deprecations for some now?)
TicketsReplaces#40783
LicenseMIT
Doc PRsymfony/symfony-docs#17064

Todo

  • Review the Psalm check of this PR
  • 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.

To avoid a PR that is too big to review and maintain, the scope of this PR is deliberately kept narrow:

  • Only PHPdoc from eitherPsalm's Symfony stubs orPHPStan's Symfony 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.

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'sEnvelope stub (due to complexity of the PHPdoc).

ro0NL, ChrisRiddell, ging-dev, alamirault, Kocal, maxhelias, Chris53897, mhujer, orklah, and zmitic reacted with heart emoji
* 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
Copy link
Member

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>

Copy link
MemberAuthor

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.

Copy link
Contributor

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>.

@stof
Copy link
Member

@wouterj you need to update the.github/expected-missing-return-types.diff file to account for the change inCommand::getHelper()

@wouterj
Copy link
MemberAuthor

This one is ready to merge now imho

Copy link
Member

@NyholmNyholm left a 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)

@wouterj
Copy link
MemberAuthor

wouterj commentedJul 23, 2022
edited
Loading

Depending on $k, this may access the array with a non scalar key. Should we modify the code and throw an exception or should we specify $k harder?

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 thatresolveString() already has a nice exception message if the array key would have been'app.%some_list%'. I've added a check and exception for this newly discovered case.

nicolas-grekas added a commit that referenced this pull requestAug 1, 2022
…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
@nicolas-grekas
Copy link
Member

Thank you@wouterj.

@nicolas-grekasnicolas-grekas merged commit1dedb22 intosymfony:6.2Aug 2, 2022
@nicolas-grekas
Copy link
Member

And thank you@mdeboer also!

mdeboer reacted with laugh emoji

@wouterjwouterj deleted the staticanalysis-stubs branchAugust 2, 2022 13:26
chalasr added a commit that referenced this pull requestNov 30, 2022
…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
PhilETaylor pushed a commit to PhilETaylor/symfony that referenced this pull requestSep 6, 2023
…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
nicolas-grekas added a commit that referenced this pull requestNov 26, 2024
…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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofstof left review comments

@derrabusderrabusderrabus left review comments

@NyholmNyholmNyholm approved these changes

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

@ycerutoycerutoAwaiting requested review from ycerutoyceruto is a code owner

+4 more reviewers

@ro0NLro0NLro0NL left review comments

@alexislefebvrealexislefebvrealexislefebvre left review comments

@VincentLangletVincentLangletVincentLanglet left review comments

@andrew-dembandrew-dembandrew-demb left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

11 participants

@wouterj@stof@nicolas-grekas@ro0NL@Nyholm@derrabus@alexislefebvre@VincentLanglet@andrew-demb@carsonbot@mdeboer

[8]ページ先頭

©2009-2025 Movatter.jp