Skip to content

Conversation

@staabm
Copy link
Contributor

@staabm staabm commented May 14, 2022

implement details mentioned in #1305 (comment) and remaining parts from #1301 (comment)

@staabm
Copy link
Contributor Author

staabm commented May 14, 2022

regarding

And similarly, if someone calls foo(1, 2) (named arguments were not used, but some required arguments are missing), we can also return null to prevent some nasty bugs in the extensions.

atm the helper returns the call-args as is, since the helper only cares about args containing named-args.
I don't see this requirement related to the named-arguments-helper - isn't this case already handled somewhere within phpstan-core?

just added a test to demonstrate the helper returns the call unchanged.


I think the helper is mostly complete as requested. next question would be on how to handle the returned null value. should it just be turned into a ErrorType on the calling sides?

@ondrejmirtes
Copy link
Member

ondrejmirtes commented May 14, 2022

This is what I meant: c7969d0?diff=unified&w=1

There's still one test failure, please fix it. And I guess it's still not called for all relevant extension types, right?

I don't see this requirement related to the named-arguments-helper

I plan to rename the class to "ArgumentsNormalizer" in the next step so the name will no longer lie about the purpose.

@staabm
Copy link
Contributor Author

staabm commented May 14, 2022

And similarly, if someone calls foo(1, 2) (named arguments were not used, but some required arguments are missing), we can also return null to prevent some nasty bugs in the extensions.

I have this change prepared locally but it still has some issues. at best we can merge here and progress in a new PR..?

@staabm staabm marked this pull request as ready for review May 14, 2022 20:58
@ondrejmirtes ondrejmirtes merged commit 4d584d1 into phpstan:1.7.x May 15, 2022
@ondrejmirtes
Copy link
Member

at best we can merge here and progress in a new PR..?

Yes :) Thank you!

@staabm staabm deleted the named-helper branch May 15, 2022 22:37
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.

3 participants