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.

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

added phpstan ConfigReturnTypeExtension #10635

Merged
merged 5 commits into from Apr 29, 2022
Merged

added phpstan ConfigReturnTypeExtension #10635

merged 5 commits into from Apr 29, 2022

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Mar 18, 2022

@staabm
Copy link
Contributor Author

staabm commented Mar 18, 2022

at this early stage, this extension already provides enough type-inference to detect type of ca3b874


 ------ ------------------------------------------------------------------------------------------------
  Line   RemoveCommand.php
 ------ ------------------------------------------------------------------------------------------------
  238    Parameter #1 $array of function array_keys expects array, array<string, bool>|null given.
  240    Parameter #1 $value of function count expects array|Countable, array<string, bool>|null given.
 ------ ------------------------------------------------------------------------------------------------

@staabm
Copy link
Contributor Author

staabm commented Mar 18, 2022

@Seldaek you are more familiar with the composer schema... are you ok with finishing the PR from this point?

feel free to ping me regarding phpstan/phpstan-extension related questions.

@Seldaek
Copy link
Member

Seldaek commented Mar 18, 2022

Ok sure, thanks!

@Seldaek Seldaek added this to the 2.3 milestone Mar 18, 2022
@Seldaek Seldaek marked this pull request as ready for review April 28, 2022 19:22
@staabm
Copy link
Contributor Author

staabm commented Apr 29, 2022

Looks promising already. I think it could make sense to ship a phpstan config with composer, meant for include in e.g. plugin projects which also use phpstan, so they also benefit from the static analysis enhancements.

if you have questions left, feel free to ask

@Seldaek
Copy link
Member

Seldaek commented Apr 29, 2022

Yeah good point, did that now, ready to merge IMO, this works great for Config! Thanks for the help getting started.

@Seldaek Seldaek merged commit 6b7ae1e into composer:main Apr 29, 2022
@staabm staabm deleted the phpstan-config branch April 29, 2022 10:09
@staabm
Copy link
Contributor Author

staabm commented Jun 2, 2022

@Seldaek not sure we missed something, but it seems atm the phpstan folder is missing from vendor/composer/composer in my plugin project, which I just updated to 2.3.6

do we need to whitelist the phpstan folder somewhere, so it is part of the composer package?

if ($addlPropType !== null) {
$types[] = new ArrayType(TypeCombinator::union(new StringType(), ...$keyNames), TypeCombinator::union($addlPropType, ...$valTypes));
} else {
$types[] = new ConstantArrayType($keyNames, $valTypes);
Copy link
Contributor

Choose a reason for hiding this comment

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

for non-required properties, the keys should be marked as optional keys (through the 4th argument)

Copy link
Contributor

@herndlm herndlm Jun 2, 2022

Choose a reason for hiding this comment

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

I can gladly open a PR adapting that. I didn't use JSON schema often yet, but I don't see many required fields there which would make almost all keys optional. I guess this is not the expected outcome, is it?
Ah and I just saw that apparently non-required props are already unioned with a NullType (still non-optional in the ConstantArray though). I'll wait before I adapt anything here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This had less impact than I thought: #10815

@stof
Copy link
Contributor

stof commented Jun 3, 2022

Btw, I'm not sure an optional key actually allows null in json schemas.

@Seldaek
Copy link
Member

Seldaek commented Jun 3, 2022

I think my point was more that if it's optional and you call get() on it it may return null.

@stof
Copy link
Contributor

stof commented Jun 3, 2022

But this is about a subkey of an array shape. You cannot call get on them

@Seldaek
Copy link
Member

Seldaek commented Jun 3, 2022

Yes I can see that, for that case I don't know.. maybe I was lost in recursion and this doesn't make sense here. I don't recall tbh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants