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

Core rector support - attempt #2 #122

Merged
merged 11 commits into from
Jul 31, 2023

Conversation

GeniJaho
Copy link
Collaborator

#120
#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
Copy link
Collaborator Author

@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
Copy link
Owner

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
cakephp/upgrade#235
https://github.com/cakephp/upgrade/pull/236/files

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

@GeniJaho
Copy link
Collaborator Author

@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
Copy link
Owner

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

@GeniJaho
Copy link
Collaborator Author

@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.

@driftingly driftingly enabled auto-merge (squash) July 31, 2023 17:53
@driftingly driftingly disabled auto-merge July 31, 2023 17:53
@driftingly driftingly merged commit ea9ebaa into driftingly:main Jul 31, 2023
4 checks passed
@GeniJaho GeniJaho deleted the core-rector-support branch August 10, 2023 14:41
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.

2 participants