driftingly / rector-laravel

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

Core rector support - attempt #2 #122

Closed GeniJaho closed 1 year ago

GeniJaho commented 1 year ago

https://github.com/driftingly/rector-laravel/issues/120 https://github.com/driftingly/rector-laravel/issues/121 This PR fixes most of the errors preventing the upgrade up to Rector 0.17.6, except for RequestStaticValidateToInjectRector, which is turning out to be a different beast entirely. I'll try to fix it in a few days, maybe in another PR.

GeniJaho commented 1 year ago

@driftingly The last commit is really a 'downgrade' of the RequestStaticValidateToInjectRector rule. It now only works when we're looking at assignment statements ($validated = request()->validate();) or direct statements (request()->validate();). The reason for that is that now we have to iterate the statements of the ClassMethods and find the request() or Request:: calls 'by hand' (the replace() method I've introduced is an abomination 😁).

I couldn't find a better way, but this solution worked in the few projects it tested it.

driftingly commented 1 year ago

Thanks for your work on this @GeniJaho I tried to figure out the required changes by what they changed in the rector codebase but couldn't. I hope they release a 1.0 soon and we can work on improving these rules.

Looks like some work was done on https://github.com/cakephp/upgrade/pull/235 https://github.com/cakephp/upgrade/pull/236/files

Might be able to use some of that for inspiration/direction.

GeniJaho commented 1 year ago

@driftingly Thank you so much for the tip, $this->traverseNodesWithCallable() did the trick. The RequestStaticValidateToInjectRector rule now works exactly the same as before, with minimal changes to the class. This branch now works with the latest release of Rector 0.17.7.

driftingly commented 1 year ago

@GeniJaho Do those skip_* tests need to be renamed?

GeniJaho commented 1 year ago

@GeniJaho Do those skip_* tests need to be renamed?

No, they were actually there before and are relevant to that Rule, the Rule should not affect usages in traits or extends. They appear as new files, but they're just modified.

Fixtures for unchanged functionality don't need to define two identical parts, like this:

<?php
$some = 'test';
?>
---
<?php
$some = 'test';
?>

If we don't put a separator at all, that tells Rector that this piece of code shouldn't be changed.

<?php
$some = 'test';
?>

So I just simplified those two fixtures, they're appearing as new files, however.