Skip to content

[Sensiolabs] Add @Template annotation to render() Rector#291

Merged
TomasVotruba merged 47 commits intomasterfrom
temlate-symfony-rector
Jan 17, 2018
Merged

[Sensiolabs] Add @Template annotation to render() Rector#291
TomasVotruba merged 47 commits intomasterfrom
temlate-symfony-rector

Conversation

@TomasVotruba
Copy link
Copy Markdown
Member Author

@mssimi Could I ask you for a review?
And also providing more use cases from your real practise?

I think we'll have to reimplement TemplateGuesser in framework agnostics way here

@mssimi
Copy link
Copy Markdown
Contributor

mssimi commented Jan 14, 2018

I will test it on my code.

@mssimi
Copy link
Copy Markdown
Contributor

mssimi commented Jan 14, 2018

I am unable to run rector of this branch. With dev-master I have no problem.

Class 'Symplify\PackageBuilder\Console\Style\SymfonyStyleFactory' not found in /home/mssimi/www/xxx/vendor-bin/rector/vendor/rector/rector/bin/rector:47

I actually added some extra commits here #292 , but dont know how to add them to this pull request

@TomasVotruba TomasVotruba force-pushed the temlate-symfony-rector branch from c8421d6 to aab3c28 Compare January 15, 2018 18:50
@TomasVotruba
Copy link
Copy Markdown
Member Author

I am unable to run rector of this branch. With dev-master I have no problem.

I've just rebased on master, there was one autoloading fix

@TomasVotruba
Copy link
Copy Markdown
Member Author

@mssimi I've added framework agnostic TemplateGuesser to cover your cases. Check it 👍

@mssimi
Copy link
Copy Markdown
Contributor

mssimi commented Jan 15, 2018

It is no good yet:)

return $this->redirectToRoute('_forgotten'); // before
return $this->render('AppBundle:User:forgotten.html.twig'); // after
// before
return $this->render('@App/User/changePassword.html.twig', array(
     'form' => $form->createView()
));
//after
return $this->render('AppBundle:User:passwordChange.html.twig');

It should touch only return array statements. For use cases where no other return statement is it seems to work well.

@mssimi
Copy link
Copy Markdown
Contributor

mssimi commented Jan 16, 2018

I checked it quickly and still found some problems, but we r getting there:). I will add more commits later today.

@mssimi I though about adding yml and other files - but it's too complicated and require lot of extra work which would influence the quality of PHP part, so I've removed it.

Damn most of my ideas require fixing yml and twig:)

Edit:

@TomasVotruba I added more tests and fixed them in my fork, but after looking more into it I found out, we will have to improve template guesser to match this one. We may have to improve bundle name guessing as well as guessing if Controller is even a bundle, since template annotation works even for non-bundle controller, where template name is generated differently.

@TomasVotruba
Copy link
Copy Markdown
Member Author

@mssimi Could you send PR with failing test cases like before?

@mssimi mssimi mentioned this pull request Jan 16, 2018
@mssimi
Copy link
Copy Markdown
Contributor

mssimi commented Jan 16, 2018

@TomasVotruba I tried to re-implement it myself, there is bug in phpdoc regeneration, it generates unneeded space, can't find it. Are you available on hangout to discuss it?

@TomasVotruba
Copy link
Copy Markdown
Member Author

Sure: tomas.vot@gmail.com

@TomasVotruba TomasVotruba changed the title [Symfony] Add @Template annotation to render() Rector [Sensiolabs] Add @Template annotation to render() Rector Jan 17, 2018
Comment thread src/config/level/symfony/symfony33.yml Outdated
'Symfony\Component\Debug\Exception\ContextErrorException': 'ErrorException'

# http-kernel
Rector\Rector\Contrib\Symfony\HttpKernel\TemplateAnnotationRector: ~
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ha, a bug :)

@TomasVotruba
Copy link
Copy Markdown
Member Author

TomasVotruba commented Jan 17, 2018

@mssimi Could you review please?

I've added support for TemplateGuesser for v5 and v3

@mssimi
Copy link
Copy Markdown
Contributor

mssimi commented Jan 17, 2018

@TomasVotruba I already refactored one project with that, everything seems fine:)

@TomasVotruba
Copy link
Copy Markdown
Member Author

I've added @Route ( space solution as well.

Thanks for fast cooperation 👍

@TomasVotruba TomasVotruba merged commit 7dfb63c into master Jan 17, 2018
@carusogabriel carusogabriel deleted the temlate-symfony-rector branch January 28, 2018 21:02
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