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

Trigger errors for unresolvable parameters #1319

Merged
merged 4 commits into from
May 17, 2022

Conversation

rvanvelzen
Copy link
Contributor

No description provided.

@@ -268,6 +268,18 @@ public function check(
))->line($argumentLine)->build();
}

if ($this->unresolvableTypeHelper->containsUnresolvableType($parameterType)) {
Copy link
Member

Choose a reason for hiding this comment

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

These contributions make me drool! So good 😊 But it should be reported only with bleedingEdge enabled (by adding new featureToggles parameter), who knows what's it gonna unleash in real codebases 😊

@rvanvelzen
Copy link
Contributor Author

The one test failure is a BC break for a non-@api method call. Should I make the message optional, or is the breakage acceptable?

@canvural
Copy link
Contributor

@rvanvelzen I think it's fine. It's my fault to use non-@api calls 😄 I'll fix it there.

@ondrejmirtes
Copy link
Member

@canvural Maybe I suggested to you myself to use FunctionCallParametersCheck so I'm sorry about it :) There should be a way in Larastan to do it nicer, just an idea: allow custom rule developer to invoke their own virtual nodes. You could invoke a custom FuncCall node and the rules in PHPStan would pick it up themselves.

But for now, feel free to prepare a new Larastan release that requires PHPStan ^1.7 (after this PR is merged), we can then coordinate the release together :)

@ondrejmirtes
Copy link
Member

Or pass the extra argument to FunctionCallParametersCheck, ignore PHPStan error for now, release it (and not change the PHPStan requirements), and clean it up after 1.7 is releaased :)

@ondrejmirtes
Copy link
Member

@rvanvelzen Just one more thing - have you looked into the failures before the new check was behind a feature toggle?

It'd be good to fix them now since it's a valuable piece of feedback about the new check :)

@rvanvelzen
Copy link
Contributor Author

@ondrejmirtes yeah, I fixed most of them in f3c5928. There's one left that I'll try to fix.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@rvanvelzen rvanvelzen force-pushed the unresolvable-parameter branch from fc3ef9b to adc4f93 Compare May 17, 2022 08:52
@rvanvelzen rvanvelzen marked this pull request as ready for review May 17, 2022 09:08
@ondrejmirtes ondrejmirtes merged commit 4f914b6 into phpstan:1.7.x May 17, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@rvanvelzen rvanvelzen deleted the unresolvable-parameter branch May 17, 2022 09:50
@ondrejmirtes
Copy link
Member

I went the optional way as that avoids all of the headaches: a114a3f

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