do not rename the template in dynamic router #388
Conversation
|
Doing so all custom controllers, should call its template parameter |
dbu
force-pushed
the
not-rename-template
branch
from
4614b4b to
1f21a67
Compare
|
they have to, if they want parameter automatch. but i think its worth it to avoid the collision with FrameworkBundle. we don't have configuration for that name, only a constant - but a parameter variable name can't use a constant afaik. however, you could write your controller expecting the Request instead, and then get the attribute with the constant... |
|
A couple of remarks here:
|
UPGRADE-2.0.md
Outdated
| ```php | ||
| public function documentAction($contentDocument, $template) { | ||
| ... | ||
| ``` |
There was a problem hiding this comment.
this file doesn't seem right. The lines 208 - 222 should be removed, right?
There was a problem hiding this comment.
indeed, messed up with copy-pasting it seems.
|
* Why at all do this renaming of |_template| to |contentTemplate|?
having a request attribute called _template triggers an exception by the
framework bundle that _template is reserved for the `@Template` annotation.
* How can this conflict with the framework's template controller?
It'll always require |template|, not |_template|.
i feel it makes sense to use the same name. its the same concept.
* Why remove |_template| when renaming it to |contentTemplate|? Does
it hurt having a duplicated request attribute?
yes, having _template triggers an exception.
* If we want to do this change, we should release a new 1.4 version
that triggers a deprecation warning when using |contentTemplate|. It
should also come with 2 attributes: |template| and |contentTemplate|
to allow forward compatibility.
putting both into the request attributes seems like a good idea. i don't
know how we could do a deprecation warning however - something in the
core of symfony looks at controller method parameters and tries to match
them with request attribute names.
|
|
Let's update this PR to use both |
|
yeah, i was going to say thats not our fault :-) |
dbu
force-pushed
the
not-rename-template
branch
3 times, most recently
from
a93071f to
65784fb
Compare
|
i think this is now ready |
src/Routing/DynamicRouter.php
Outdated
| * This name is deprecated in favor of "template" for consistency with | ||
| * FrameworkBundle and will be removed in RoutingBundle 3. | ||
| */ | ||
| const BC_CONTENT_TEMPLATE = 'contentTemplate'; |
There was a problem hiding this comment.
I don't think we have to use a constant for the BC version, it'll only cause more conflicts (people shouldn't start using this constant)
wouterj
force-pushed
the
not-rename-template
branch
from
65784fb to
d46874e
Compare
|
Applied my comment and slightly tweaked the changelog and upgrade guide. Feel free to merge if you agree with my changes. |
$template is in line with the FrameworkBundle TemplateController
wouterj
force-pushed
the
not-rename-template
branch
from
d46874e to
8ebb686
Compare
|
good point, thanks for the cleanup and the merge! |
$template is in line with the FrameworkBundle TemplateController
together with symfony-cmf/content-bundle#174