[WIP] Add Twig functions for cmf_document_path and cmf_document_url

[benglass]

Discussion in #239

[benglass]

@dbu Thoughts? I don't fully grok the isUrlGenerationSafe code in the main twig extension, seems to be concerned with skipping escaping html characters in the url under certain circumstances for performance reasons so I dont think its critical here. In order to leverage that code without copy pasting this class would have to extend it it seems?

[dbu]

i would extend/instantiate https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Extension/RoutingExtension.php directly in this class and forward getObjectPath to it with a try-catch around the getPath call. then we are sure we do exactly the same and avoid any duplication.

[dantleech]

This is very PHPCR specific .. i.e. cmf_document_path .. what about the ORM?

cmf_object_path($fqn, $id, $parameters, $relative) ?

This would fit the semantics of ObjectManagerInterface::find()

But then IMO this should really be in a PHPCR helper.

[wouterj]

Besides that this is PHPCR specific, I don't see what makes this different than the path and url functions provided by the TwigBridge? If it's only about the method name, I'm -1 (and if it's only by the method name, you should wrap the TwigBridge extension instead of replicating it).

Also, you need to create a templating helper. See the CoreBundle and BlockBundle on how to do this.

[benglass]

@wouterj @dantleech The reasons for this PR are described in #239

The existing path and url twig functions do handle cmf document paths but they also throw RouteNotFoundException and cmf paths are often editable by clients whereas normal symfony route names are not. Therefore it came out in discussion that it would be nice to NOT assume we want to throw a RouteNotFoundException inside a twig template and cause a 404 or 500 error because a client moved or deleted a document.

@dantleech Regarding orm objects I'm not sure I'm clear on how we would generate URLs for arbitrary ORM objects? Is this handled by the url generator somehow?

@wouterj I can look at creating a templating helper if we decide to move forward on this.

@dbu If you wouldnt mind commenting on Wouter and dantleech's concerns

[wouterj]

cmf paths are often editable by clients whereas normal symfony route names are not.

Whether a name changes a lot (I'm not sure of that) or not, if it doesn't exists it is still an error imo.

cause a 404 or 500 error in a template because a client moved or deleted a document.

This should instead be done in a kernel.exception listener then.

Regarding orm objects I'm not sure I'm clear on how we would generate URLs for arbitrary ORM objects? Is this handled by the url generator somehow?

If the ORM support of Routing is 100% complete (I don't think so :P), it is all handled by the generator. The problem is that the function name is ODM specific (usage of document).

[benglass]

I don't disagree with you that it is an error but I do not think its ok for the template to be a 404 error because one of the links on it is to a document that doesnt exist. @dbu and I have discussed that if you expect the path to change you should probably create a named symfony route and use that instead however the fact that use of the twig path function works with cmf paths and that cmf paths can be modified by end users and that this can cause a page in which there is a link to a cmf document to become a 404 does not seem like ideal behavior. This problem doesnt exist with using path('route_name') because clients cannot generally change symfony routes so it is unique to the dynamic router.

Is it possible to tell in a kernel.exception whether the RouteNotFoundException is the result of the URL not matching in the request (someone tries to visit http://www.domain.com/bad/route) which should remain a 404 or a call to UrlGeneratorInterface::generate? If so I completely agree that an optional kernel.exception listener would be preferred. Then it could just be turned on in production, for example. If this option is available then I think we can get rid of the twig approach entirely which I agree would be an improvement because users would not have to know to use a different twig function for cmf documents.

[dbu]

@benglass thanks. i will look at the changes. please use the PR template as explained in https://github.com/symfony-cmf/RoutingBundle/blob/master/CONTRIBUTING.md

about the naming. indeed cmf_object_path and cmf_object_url would be more neutral and thus the better name.

@wouterj the point we want this is that if you have a mere content error - say we have a document containing a list of children documents that should have a route to them, but one of them has not - we probably want the site to still render correctly. having a 500 page is a really big issue in a production site, and e.g. sonata admin can not prevent a document being attached.

so the idea, as discussed in #239 and i think some time ago on the mailinglist, is to provide the tools to the template designer to say he wants a link if possible, but no failure if not.

i just noticed that we have cmf_is_linkable which tries to guess if a document can be linked to (right interface and actually has at least one route) https://github.com/symfony-cmf/symfony-cmf-docs/blob/dev/bundles/core/templating.rst - but if there is a missing parameter or some other reason things don't work out, it is less save. so i still feel it makes sense to do this. given that cmf_is_linkable is in the CoreBundle, i wonder if we should add this templating function there as well. (and we said we support the php templating as well, so we would need to do the same as in core bundle).

[dbu]

maybe something like "content route not found"?

[lsmith77]

ping

[dbu]

@lsmith77 there is open discussion that went to sleep:

• do the functions make sense or is cmf_is_linkable enough? maybe with an extra parameter to check if a RouteReferrersInterface object really has routes attached or only an empty list. or a fix that does BC break on is linkable.
• if we think its convenient to have the functions, what would be the right name? and what would be the right repository - here or CoreBundle?

thinking about it again, i guess cmf_is_linkable should check if it will be safe to call path/url with an object, so also check if there is a route attached. then we don't need this function, as often you would want to also not do an <a href=" on something that is no link, or omit the whole text. so a failsafe url generation function is not that helpful.

[benglass]

I dont know that this particular PR (adding these twig functions) would address this issue but we have been running into the problem of needing to add links in a twig layout (the main layout) which link to dynamic cmf documents (for example a Privacy Policy document link in the footer to /cms/main/privacy-policy). Since privacy policy is a dynamic document and editable by clients, using path('/cms/main/privacy-policy') is not a good option since it could change and then every page of the site would break due to RouteNotFoundException thrown by the twig path/url functions.

We currently try to handle this by using uuids when possible since they are less volatile, but we still have to use our own cms_path/cms_url functions which log RouteNotFoundException instead of throwing them.

I think the ideal solution would be to come up with a way to create named routes (like privacy_policy) which resolve as if they were cmf routes ('/cms/main/privacy-policy') so that you can create 'shortcuts' or 'aliases' to important cms routes that are not subject to change. I had emailed the list about this and got a response that it wasn't really possible right now.

This is just a brain dump of why I originally opened this issue.

[dbu]

that is an interesting problem that occurs with combining templates and applications with cms content. i have seen the idea of one document at a known location (e.g. the home, like /cms/content) that links to all the required special pages. this is however still tricky.

having some check if a document can be linked is not really a solution but a workaround. i think what we do need are:

• a way to find a page by "type" so we can assign a page a role (like "terms & conditions" or "contact information") and a twig function to link to a page with a role. one tricky bit could be to avoid multiple pages with the same role - except in multisite setups where each site might have one of each role...
• optionally have a security voter to prevent users from removing the active page with a specific role.

i think the role should be on the content: this avoids confusion with multilang, and the role is an information about the content. we could try to think of a way to bring in roles for routes however, like a map of role to routename, so i could transparently switch the contact page to a static route that handles a symfony form. but i think that functionality would belong into ContentBundle as its really a "content" concept.

[dbu]

i just noticed that CmfHelper::isLinkable already checks if a content returns > 0 routes for getRoutes. so i think nothing additional needs to be on that side.

@benglass are you ok if we close this PR? if you want to further discuss the role idea, we should open an issue on ContentBundle.

[benglass]

@dbu I see what you are saying and I think it may be a good approach to this issue but fundamentally we are talking about routes as opposed to content and having a way to link to them using a meaningful and immutable identifier. The path does not meet this criteria because it is not immutable and the uuid doesnt because it is not meaningful and may differ from one installation to another (could be solved with parameters but that is not convenient).

How about adding something to a route as you are saying that would provide a 'slug' or something like the uniquely identifiable type field which would work with the router?

Perhaps this could be used for both content and routes (loading either via path or uuid presents problems listed above) via a common interface for having an immutable slug? In terms of multisite setups you could just place the burden on the user to prefix the slug name with something specific to the website (eg main-contact vs intranet-contact).

[benglass]

One thought would be to provide a RouterLoader that loads statically named routes for any routes with the types/slug field. It would simply load all documents with a value for this field and use this field as a route name with all options coming from the dynamic route itself (host, method, etc would come from the route).

[dantleech]

+1 for something like {{ path('my_dynamic_route') }}, e.g. {{ path('homepage' }} that would generate a route for the homepage content.

The problem is maintaining the uniqueness of the role identifiers.

Essentially I don't think this can be done cleanly or efficiently on the routes themselves. It would be better to have a node/document which acted as a route role registry, e.g.

jcr:primaryType: nt:unstructured
homepage: <<uuid>>
contact: <<uuid>>

Where the UUID is either a content or a route.

Such a document/node could be part of a Site/Configuration document at the base of the content tree.

We could:

• Register a router (or a RouteProvider?)
• Initialize the "roles" in configuration
• (if this were a bundle) provide a dev feature to allow the selection of a target when the role has not been assigned to a target.
• or if in prod mode, crash with an exception if ANY of the roles are unassigned (as we will always have to read the node when the router is queried)

[dbu]

i think i like dans idea of a central node / document that contains this
name=>route map. this is the most flexible for multisite situations
(even allowing fallback in a well defined order) and allows for easy
validation. for validation, i think we should have the options of
required and optional pages. maybe having an "about me" page is optional...

[dbu]

looks like this was the starting point for the whole Resource / ResourceBundle... but what do we do about those two twig functions? i still think we should have them.