Skip to content
This repository has been archived by the owner on Feb 24, 2023. It is now read-only.

Specifying templates directly in Routes #266

Closed

Conversation

lemoinem
Copy link

@lemoinem lemoinem commented Jan 7, 2014

This allows using a _template attribute in the route directly to specify a template, just as if a @Template annotation was used.

Disclaimer: This PR is basically a reimplementation of #253 I tried to reach the initial author of the PR (@ewgRa), but to no avail.

I re-implemented the PR because my solution is much simpler (8 lines) and avoid declaring a new EventSubscriber.
I also took the occasion to write some documentation for it.

If specified through the route, the _template parameter will be removed from _route_params.

This PR contains an additional feature with respect to the original one: If true is specified as the value for _template, then the TemplateListener will use the Template auto guessing. I though this might be useful when Routes are generated using PHP or another configuration loader supporting literals.
This allow us to avoid having to insert an additional use Template and @Template annotation in very simple cases.

Moreover, if an invalid value is specified for _template, the listener will simply ignore it (as opposed to crashing before).

@ewgRa
Copy link

ewgRa commented Jan 7, 2014

I close my PR, haven't much time to finish it and your one seems more right and simple. Maybe there is good point to write test for this new feature.

And just one thing: why return at "else"? In this case old code will be not work.

@lemoinem
Copy link
Author

lemoinem commented Jan 7, 2014

Yes, as mentioned in the other PR, I didn't write tests because I didn't see where put them.

The else was indeed me going a bit too fast. I rewrote the if to do what I intended to.

@ewgRa
Copy link

ewgRa commented Feb 6, 2014

I test it, seems it works, but seems some tests not passed.
I think when you fix it and squash your work to one commit, you can drop WIP from this PR.

I test this PR on this cases: https://gist.github.com/ewgRa/abbaac259c750d92e8a5
And with this methods: https://gist.github.com/ewgRa/1cc6438aed899c0c3f33

I think if this PR will be accepted, this test cases can be implemented via some functional tests. But this make sense only if we know that PR will be accepted.

+1 to this PR

@ewgRa ewgRa mentioned this pull request Feb 6, 2014
@lemoinem
Copy link
Author

lemoinem commented Feb 6, 2014

I just rebased on top of current master.

Failing tests are not related (all concerning EventListener\HttpCacheListener).

I fixed the fabbot coding standard typo.

I would very much like to keep the WIP, at least until someone with the commit bit (e.g. @fabpot) gives us feedback on the PR, such as would it be merged with tests and what tests could be implemented or something like that (and once some tests have been written).

Thanks for your help and feedback on the PR, helped a lot. If you have suggestion regarding the tests, or feel like implementing them yourself, be my guest!

@fabpot
Copy link
Member

fabpot commented Jul 8, 2014

I don't like this. It mixes different responsibilities in the routing configuration and adds yet another way to do something that we can easily do with the @template annotation.

@fabpot fabpot closed this Jul 8, 2014
@lemoinem
Copy link
Author

lemoinem commented Apr 8, 2015

Thanks @fabpot for the feedback. I understand not every feature need to be included.

However, for the ones interested in the feature, you can find it in its own bundle: https://github.com/wemakecustom/TemplateInRouteBundle

@lemoinem lemoinem changed the title [WIP] Specifying templates directly in Routes Specifying templates directly in Routes Apr 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants