rectorphp / rector

Instant Upgrades and Automated Refactoring of any PHP 5.3+ code
https://getrector.com
MIT License
8.63k stars 679 forks source link

Allow void return type for non final protected methods #7802

Closed phh closed 1 year ago

phh commented 1 year ago

Feature Request

Migrating to new latest Laravel 10 version manually I decided to look into if Rector could help out here. Lots of the skeleton methods has been changed from using docblock types to real return types.

Like this:

image

However, it did not work. But it did when I made the class final!

Dug into the tests and it seems like it's very intentional: https://github.com/rectorphp/rector-src/blob/beb6e9127c74b5844dc6165cd60cba9c7136a411/rules-tests/TypeDeclaration/Rector/ClassMethod/AddVoidReturnTypeWhereNoReturnRector/Fixture/skip_non_final_protected_method.php.inc

But to me I really don't care if the method is protected and the class is not final (which is just how Laravel does it).

So a feature request could be to allow an option to disable the isProtected() and isInsideFinalClass() check in AddVoidReturnTypeWhereNoReturnRector

Diff

-protected function schedule(Schedule $schedule)
+protected function schedule(Schedule $schedule): void
TomasVotruba commented 1 year ago

Hi, this could be BC break if another class extends yours. That's why it's skipped.

When you make your class final (not the vendor one), it should add the void as safe

phh commented 1 year ago

Hi, this could be BC break if another class extends yours. That's why it's skipped.

When you make your class final (not the vendor one), it should add the void as safe

Yes, this makes sense in general, but not so in a Laravel context and how we try to avoid final.

... So adding an exception around this is not an option? That would be me adding my own custom rule?

TomasVotruba commented 1 year ago

This is not related to Laravel, I use all my classes final in Laravel projects.

What error you get when you make this class final?

phh commented 1 year ago

This is not related to Laravel, I use all my classes final in Laravel projects.

What error you get when you make this class final?

No error. Just a matter of preference and code style to add final or not :)

TomasVotruba commented 1 year ago

I see :) In that case you can use coding standard to clean up. Rector's job is to be reliable, and adding breaking types had caused serious bugs in the past.