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

Add new rule to add annotations from router configuration to symfony controllers #169

Merged

Conversation

malteschlueter
Copy link
Contributor

See #168

@TomasVotruba
Copy link
Member

Btw, I've triggered the CI for feedback. Could you check before review?

use Symfony\Component\Routing\RouterInterface;

/**
* This command must be added to the application to export the routes to work with the "AddRouteAnnotationRector" rule
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There exists some files with information about routes inside the var/cache directory but the information is not direct accessible and the controllers are not resolved. There still stand FooBundle:Bar:baz.

So I tried to use the symfony command debug:router --format=json but there are also the controllers not resolved.

But if you use this little command inside the application it resolves the controllers correct.

TBD

Copy link
Member

Choose a reason for hiding this comment

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

Maybe here you can use this little trick, to fetch router directly from the project container:
matthiasnoback/phpstan-twig-analysis@dc2a650#diff-129286493c2f657f968564746944f9686bf92d5da34d86fd1b78daac315137beR25-R28

Not sure if it will provide all resolved routes though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could work but I'm failing :/ Maybe that can do someone smarter than me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried on the weekend a little bit more but without any success. The one problem I've is to run the new rector code inside my project. I can't install the source repository because of the dependencies. Only rectorphp/rector works but it hasn't the new code because it's not merged.^^

So I tried to install it in another directory but then it worked not correctly. Don't know if the autoloader of the project was missing or so.

I've tried to do some other worksaround but without success.

At the end I tried to include the var/cache/dev/AppDevDebugProjectContainer.php file in the working test https://github.com/rectorphp/rector-symfony/pull/169/files#diff-b2f0ce8a47a7ff2391eee06b6e777b9ff3829385d00695740c4793bc746cf5b4 but then I've got fatal errors because the classes and methods were not correct as the interfaces say. I think because the project runs with SF 3.4 and rector with SF 5.4.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks for trying all the options. We can merge this PR, if CI is passing. The code does not have to be complete.

Could you share a minimal reproducible repository with your setup?
I see what can we do.

Copy link
Member

Choose a reason for hiding this comment

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

PHPStan says it doesn't know Symfony\Component\Routing\RouterInterface::getRouteCollection() but the component is installed. See rectorphp/rector-symfony/runs/6442259714?check_suite_focus=true#step:4:124
How does I solve it?

You can skip this file in phpstan.neon, as it requires external context

Copy link
Member

Choose a reason for hiding this comment

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

And thank you very much for your help @TomasVotruba :)

You're welcome 👍

I have no doubts, we'll finish it this week to test it out on edge-cases. You've done great works so far, thank you 🙇

Copy link
Member

Choose a reason for hiding this comment

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

I'm in the https://github.com/malteschlueter/symfony-reproducers/tree/feature/rector-add-route-annotation in rector-add-route-annotation branch. But when I run:

vendor/bin/rector

it's missing the package. What exact steps should I do to make Rector fail?

Copy link
Member

Choose a reason for hiding this comment

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

Btw, after Rector + PHPStan fix in CI, we can merge it to progress with testing on rector/rector-dev-main 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in the https://github.com/malteschlueter/symfony-reproducers/tree/feature/rector-add-route-annotation in rector-add-route-annotation branch. But when I run:

vendor/bin/rector

it's missing the package. What exact steps should I do to make Rector fail?

I haven't installed it because I can't get my rectorphp/rector-symfony feature branch to run/installed.

Because of that I changed in the rectorphp/rector-symfony the test configuration like I sayed in #169 (comment)

@malteschlueter
Copy link
Contributor Author

malteschlueter commented May 13, 2022

Btw, I've triggered the CI for feedback. Could you check before review?

The PHPUnit fails now only because of the new test I've added where you see the quote issue.

I will fix the PHPStan errors later

@TomasVotruba TomasVotruba marked this pull request as ready for review May 16, 2022 16:12
@TomasVotruba
Copy link
Member

TomasVotruba commented May 16, 2022

Could you rebase on dev-main? I've cherry picked 2 changes to make this PR easier to review.

@TomasVotruba TomasVotruba merged commit 7b40c37 into rectorphp:main May 16, 2022
@TomasVotruba
Copy link
Member

Let's merge this 👍

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.

None yet

2 participants