symfony-cmf / routing-bundle

Symfony bundle to provide the CMF chain router to handle multiple routers, and the dynamic router to load routes from a database or other sources.
161 stars 78 forks source link

Add Twig functions for cmf_document_path and cmf_document_url #239

Open benglass opened 10 years ago

benglass commented 10 years ago

Moved from https://github.com/symfony-cmf/Routing/issues/90

Currently there is support for generating URLs for cmf documents using the symfony path and url twig functions but it would be nice to have a dedicated function that only generates CMF-based routes for the following reasons:

It might be better NOT to throw a RouteNotFoundException for these kind of routes because they are volatile (user's can often edit them and move them) and having this throw exceptions to the template is not desired

In a multi-domain situation these functions would ensure that routes being generated for a site other than the current one would always be generated as absolute although this logic would perhaps live in a DomainAwareGenerator

There is discussion happening around this here symfony-cmf/RoutingBundle#178 but there is nothing stopping us from creating these functions using the current route generator rather than the chain route generator and also implementing the don't throw exceptions approach.

How should exceptions be handled? You could have a $throw boolean method in the constructor for the twig extension that is set based on the kernel.debug parameter (so it would throw exceptions in dev but not production. This might not be ideal because it would make it hard to work on a page that was throwing exceptions. Perhaps it could just log these exceptions in dev as critical errors.

benglass commented 10 years ago

@dbu do you still feel it is correct have these functions catch and log exceptions?

I am wondering if this is inconsistent with the symfony routing component.

The reasoning for me was that if you use twig functions in a cms then the documents can be moved and its likely that the paths may change and result in this kind of error. For us the solution is the have our own custom cms_path and cms_url functions which catch and log these errors. Normal routes can change as well and twig still throws an exception for those so just putting this logic into the cmf versions of the twig functions only solves it for those routes and introduces some inconsistency.

For our own implementation I am leaning towards having our cms_path/url twig methods the purposes of which are to catch/log errors and not introducing a separate twig function just for cmf documents. I dont think this is necessary especially if the url generator/route can handle generation for routes outside the current host (which it should be able to using the new candidates strategy to support multiple basepaths combined with a doctrine listener on load for cmf docs that sets their host requirements properly).

Interested to hear your thoughts

ElectricMaxxx commented 10 years ago

I do not really follow the complete discussion, but i can offer an other solution: https://github.com/symfony-cmf/SeoBundle/issues/1 This was my very first issue/enhancement on SeoBundle and it is important for a CMS to have such a handling and it should be the task of the SeoBundle to generate answers for search engines and end users if a route couldn't be found. In my case it shouldn't matter what the reason for that NotFoundRoute is, but the answer does matter: 404 and some extra information for the user. That means SEO and UX in one. (Btw i won't be able to add that for 1.0 that's planed for 1.1 of SeoBundle)

dbu commented 10 years ago

@benglass i think it does make sense. its rather special cases, as you should not need to hardcode something like path('/cms/routes/en/test') in your template (since having worked with drupal, i am really wary of mixing content and application. if you need this route to exist and deploy it, make it a core symfony route). however, say i have a list of documents that is supposed to have a route pointing to them, and i loop over them. now if accidentally one has no route, that should not break the application.

so i think having those twig functions (basically wrappers with a try-catch around the standard functions) makes sense. we should log the situation as an error, its not something supposed to happen casually but its a data problem. if people use it to generate their core routes with this, its their own problem, we can't prevent that. one question is what to return in that case - i guess a configurable url that defaults to "/" would make most sense.

@ElectricMaxxx a nice 404 page is another piece to the puzzle. but here the issue is when generating routes.