Navigation Menu

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

Fix PHPStan error reported for ModelHydrator::hydrate (and tighten $class param type for all hydrator classes) #771

Conversation

ZebulanStanphill
Copy link
Contributor

This is just a simple little PR that addresses the following message you get when you run PHPStan's static analysis on the project:

 ------ ------------------------------------------------------------------------------------------------------ 
  Line   Hydrator/ModelHydrator.php                                                                            
 ------ ------------------------------------------------------------------------------------------------------ 
  45     Parameter #1 $callback of function call_user_func expects callable(): mixed, non-empty-string given.  
 ------ ------------------------------------------------------------------------------------------------------

image

The fix just changes the callable to use the array syntax for static methods seen in the official PHP docs.

I also added an additional PHPStan class-string type annotation to make it clearer for PHPStan users that the function expects class name strings.

Line 45: Parameter mailgun#1 $callback of function call_user_func expects callable(): mixed, non-empty-string given.
@ZebulanStanphill
Copy link
Contributor Author

Psalm caught an issue resulting from my changes:

Error: Argument 2 of Mailgun\Hydrator\ModelHydrator::hydrate has the more specific type 'class-string', expecting 'string' as defined by Mailgun\Hydrator\Hydrator::hydrate

image

So I went ahead and fixed that by just annotating all the hydrator classes with class-string for the $class parameter type.

@ZebulanStanphill ZebulanStanphill changed the title Fix PHPStan error reported for ModelHydrator::hydrate. Fix PHPStan error reported for ModelHydrator::hydrate (and update $class param type for all hydrator classes) Aug 5, 2021
@ZebulanStanphill ZebulanStanphill changed the title Fix PHPStan error reported for ModelHydrator::hydrate (and update $class param type for all hydrator classes) Fix PHPStan error reported for ModelHydrator::hydrate (and tighten $class param type for all hydrator classes) Aug 5, 2021
… by Psalm.

The issue caught by Psalm:

Argument 2 of Mailgun\Hydrator\ModelHydrator::hydrate has the more specific type 'class-string', expecting 'string' as defined by Mailgun\Hydrator\Hydrator::hydrate
@ZebulanStanphill
Copy link
Contributor Author

Ended up having to tighten the types of some of the methods that were calling Hydrator::hydrate, and I also fixed a mistake I made in the prior commit where I put an annotation in the wrong docblock.

@Nyholm
Copy link
Collaborator

Nyholm commented Aug 18, 2021

Thank you.

Some libraries dont want to "pollute" their code base with non-standard PHP docs. I also know that there are tonnes of valid PHP code that static analyzers do not understand. And I dont really want to rewrite good bug free code just to please a non-standard tool.

That said, PHPStorm recently added features for generics. If these changes will help a developer (not phpstan/psalm), then I'll be very happy to include a version of this.


Im happy with the current changes. Will they allow auto completion with the latest version of PHPStorm? Could we help PHPStorm by adding a return type?

@ZebulanStanphill
Copy link
Contributor Author

I created this PR because while using the Mailgun PHP library, I ran into some type warnings reported by PHPStan, which were caused by some incomplete/vague types. I use PHPStan for my own project's static analysis, and I noticed this project had a PHPStan config file as well, so I figured I'd take a stab at fixing the issue myself.

So I began working on a PR to try and fix the type issues I noticed. The first PR involved the use of generics to tighten the types surrounding Hydrator, HttpAPI, and etc. But while working on it, I found that PHPStan was already reporting some issues in the project before I'd even made any changes, so I figured that I should probably fix those first before trying to tackle anything else... otherwise, it could get confusing to keep track of which issues are being caused by what.

Psalm (which this project also has a config file for) was reporting a whole ton of issues as well, so I was planning to create a PR to address that as well in the future.

I just realized that Psalm also supports the class-string pseudo-type, so I could easily fix this type vagueness for Psalm as well, but there's a bit of a catch:

PHPStan, Psalm, and even phpDocumentor all support @param class-string in doc blocks (so in that sense it's a "standard"), but PHPStorm currently does not, unless you install an extension. So there's 2 options here:

Option A: Replace the current @phpstan-param class-string in my PR with @param class-string, which would work with both PHPStan and Psalm. However, you'd have to use something like the previously-linked extension when using PHPStorm (if you're not already); otherwise PHPStorm would likely be confused by the class-string type.

Option B: Just use both @phpstan-param class-string and @psalm-param class-string. This would support PHPStan and Psalm, while also not interfering with an IDE (be it PHPStorm, VS Code, or anything else) that don't natively support the class-string pseudo-type. The downside is having to use 2 annotations for the param types instead of 1, which feels kinda bloated.

As for how useful the types in this PR actually are: besides clearing the slate of reported errors to make working on future type enhancements easier, I'm not sure. There is some clear use-cases for the class-string type as seen in one of PHPStan's documented examples, but in this particular case, the methods in use appear to be mostly just used internally by this library. So I think they're mostly just useful for preventing regressions caused by future changes to this library.

@Nyholm
Copy link
Collaborator

Nyholm commented Aug 18, 2021

Thank you for this well descriptive answer.

I would be much more happy with @param class-string as it it "standard" compared to @phpstan-param class-string and @psalm-param class-string. Ie, it is not tied to a specific tool.

I will put my money on that PHPStorm will support that within the near future, in the mean time, one can use the plugin as you say.

I would prefer option A.

@ZebulanStanphill
Copy link
Contributor Author

Alright, pushed a commit to switch to @param.

Copy link
Collaborator

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

Copy link
Collaborator

@DavidGarciaCat DavidGarciaCat left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution

@DavidGarciaCat DavidGarciaCat merged commit 6f678bf into mailgun:master Aug 20, 2021
@ZebulanStanphill ZebulanStanphill deleted the fix/model-hydrator-phpstan-error branch August 26, 2021 18:12
@ZebulanStanphill
Copy link
Contributor Author

Just wanted to note that I recently discovered that the PhpStorm extensions for Psalm and PHPStan are the official support.

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

Successfully merging this pull request may close these issues.

None yet

3 participants