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

Prevent exceptions on UnionTypes #2

Merged
merged 4 commits into from
Aug 18, 2022

Conversation

pascalheidmann
Copy link
Contributor

If you have a codebase with functions that return UnionTypes this rector sadly will fail with a fatal error

Fatal error: Uncaught Error: Call to undefined method PHPStan\Type\UnionType::getClassName() in vendor/codito/rector-money/src/Rule/CurrencyAvailableWithinToCurrenciesContainsRector.php:55

 Fatal error: Call to undefined method PHPStan\Type\UnionType::getClassName() in vendor/codito/rector-money/src/Rule/MultiplyAndDivideByStringRector.php:129

This PR won't do magic like checking each type in its union but at least allow the rector to work as intended on simpler code without variable return types.

@Wirone
Copy link
Contributor

Wirone commented May 30, 2022

Good to see someone actually used this package 🥳 I'll look at it in the evening, thanks for the contribution anyway!

@Wirone Wirone self-assigned this May 30, 2022
@Wirone Wirone force-pushed the prevent-non-class-exceptions branch from 191aa80 to 2f305fd Compare May 30, 2022 21:27
@Wirone
Copy link
Contributor

Wirone commented May 30, 2022

@pascalheidmann I've rebased your branch on top of main, which was updated with some changes in dev requirements (newer Rector, ECS, PHPStan). Now checks look better, there's only one CS issue (unused import) that must be fixed.

@Wirone Wirone force-pushed the prevent-non-class-exceptions branch from 2f305fd to 8f0d9c5 Compare May 30, 2022 21:59
@Wirone Wirone force-pushed the prevent-non-class-exceptions branch from 8f0d9c5 to b11d3bf Compare May 30, 2022 22:25
@pascalheidmann pascalheidmann force-pushed the prevent-non-class-exceptions branch from 9fa6dcf to 0d0f44a Compare May 31, 2022 07:40
@pascalheidmann pascalheidmann requested a review from Wirone May 31, 2022 07:41
Copy link
Contributor

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

(see comment below, did not check proper review option, I'm used to Gitlab more)

Copy link
Contributor

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

Please provide ignore test fixture for scenario (like tests/Rule/MultiplyAndDivideByStringRector/Fixture/skip_int_or_string_multiplier_and_divider.php.inc) which will cover your changes 🙂

@pascalheidmann pascalheidmann force-pushed the prevent-non-class-exceptions branch from 0d0f44a to 7037383 Compare June 24, 2022 12:06
@pascalheidmann
Copy link
Contributor Author

@Wirone I added a testcase to each rule in which no changes are detected. Hope those fit.
Maybe I find some time to add proper union type handling inside a new PR ;)

Copy link
Contributor

@Wirone Wirone 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 continuing your work 🍻 Even though I find those scenarios weird (PHPStan would complain about them most probably since multiply() or divide() could be invoked on non-Money values) I think this fix is needed 👍

BTW. I just found out that probably I need to change how floats passed directly as an argument are handled - these should be wrapped with sprintf() too. WDYT?

@pascalheidmann pascalheidmann requested a review from Wirone July 20, 2022 15:48
@Wirone Wirone force-pushed the prevent-non-class-exceptions branch from 639ff4c to 0273974 Compare August 18, 2022 13:47
@Wirone
Copy link
Contributor

Wirone commented Aug 18, 2022

Thank you @pascalheidmann 🍻

FYI: I've "fixed" errors for PHPStan in main branch, so actions here are green. I'm not fully satisfied with how it was done and I will try to improve it, but your changes can be merged anyway 🙂

@Wirone Wirone merged commit 43d7300 into Codito-Dev:main Aug 18, 2022
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