driftingly / rector-laravel

Rector upgrades rules for Laravel
http://getrector.org
MIT License
540 stars 52 forks source link

Add ResponseHelperCallToJsonResponseRector #252

Closed Stoffo closed 2 weeks ago

peterfox commented 3 weeks ago

It might be better to scale this up to be a rule to handle all response helper scenarios.

Or to make a new configurable rule for the main rector repo. FuncCallToMethodCallRector kind of does the same thing but it instead injects a property into a class via the constructor. I'd say a MethodCallToNew might make sense.

GeniJaho commented 2 weeks ago

It might be better to scale this up to be a rule to handle all response helper scenarios.

I don't think it's possible to add them all under one configurable rule, I suspect other response() methods require more than just passing parameters to the new constructor. We could add them in other rules, and make a set out of it 👀

Or to make a new configurable rule for the main rector repo.

A new MethodCallToNew in the core repo would probably do the trick, but I'm not sure it can be done in a way that handles different use cases. We have a simple one here, just copy the method params into the class constructor, but how useful/common would that Rector be for other use cases? However, this makes me think we need some core helper rules for common transforms like these...

@peterfox so how do you want to handle this PR? 😅

peterfox commented 2 weeks ago

@GeniJaho I think for now it's not a bad rule. Let's get it in and then if we find it doesn't scale we'll deprecate this for something else.