Commit patch: #74
Last updated: #101
Problem/Motivation
When trying to generate a URL to a Drupal page in a template, it would be preferable to use the route name (the immutable machine name) rather than a system path that may be altered if the route path pattern is altered.
Proposed resolution
The theme system would like to have one consistent way of creating urls for pages:
- Retrieving a URL to a page from a "path" is deprecated in D8. We should not use them. Period.
- Provide one consistent way to create URLs for pages, using routes.
- Repurpose the
{{ url() }}
Twig function to be more in line with Symfony's, using routes. This outputs an absolute URL for the given route. - Add a
{{ path() }}
Twig function, similar to Symfony's. This outputs a relative URL for the given route. - This will ensure people learn the correct way (routes) to create URLs for pages and Symfony people will already know how to do it.
- Fix a bug in the CLI test runner and makes the URL generator logic more robust in case someone sets an empty port value. This fix is included since otherwise the twig test fails.
- Create follow up issues (done, see below) to make creation of routes simpler by adding a route discovery mechanism.
- Create a follow up issue to introduce a system provided
{{ base_path }}
variable to assist for novice users to create manual links when needed.
@see http://symfony.com/doc/current/book/routing.html#generating-urls-from-a-...
Remaining tasks
None
User interface changes
None
API changes
D8 API change (not D7 -> D8):
The original {{ url() }}
Twig function that was initially added (but scheduled for removal), will be changed to take:
- A route, parameters and options.
- Will always output an absolute URL. Use
{{ path() }}
for relative URLs.
The impact is minimal since creating URLs in templates does not exist in core, we use preprocess functions to introduce the used variables in templates.
Related Issues
Follow-ups:
#2106811: Create route discoverability (back-end, method/function)
#2106797: Create a Twig function for route discoverability (front-end, twig extension)
#2106859: Add a link generator Twig extension
Pros / Cons
Pro
- Routes are now to be used; paths are dead in Drupal.
- Consistency is more important than hundred or even 2 ways to create the same type of output.
- We want to be compatible with at least one system, in this case Symfony.
- This is for advanced use cases when themers want to create links to content outside of the template's scope. This seldomly occurs in in templates.
- A novice themer would use the '/my-url' link directly; once he wants more he is able to C&P a route example from a tutorial.
Cons
- It should be possible for a novice themer coming from D7 to create a link within a template by just knowing the path from the URL.
Comment | File | Size | Author |
---|---|---|---|
#170 | interdiff.txt | 464 bytes | dawehner |
#170 | twig_url-2073811-171.patch | 17.81 KB | dawehner |
#169 | 2073811-FAIL.patch | 18.26 KB | dawehner |
#167 | add_a_url_generator-2073811-167.patch | 18.26 KB | joelpittet |
#167 | interdiff.txt | 954 bytes | joelpittet |
Comments
Comment #1
dawehnerI need some help with as this does not work yet.
Comment #2
dawehnerThis was way too much.
Comment #4
pwolanin CreditAttribution: pwolanin commentedGot this working by shifting to setter injection (no idea why that's needed) and with some help from Cottser in IRC and finally noticing that the method didn't have 'return'.
Make for a working test too.
Comment #5.0
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #6
pwolanin CreditAttribution: pwolanin commentedoops - can't access those services during install - so absolutely need to use sett injection in any case.
Comment #8
pwolanin CreditAttribution: pwolanin commentedsilly me - the tests run in a subdir. Let's just use the proper generators, even though it's a bit of a silly test.
Comment #10
dawehnerI need some debug informations from the testbot.
Comment #11
pwolanin CreditAttribution: pwolanin commentedThe generated URL in the test result is "http://drupaltestbot1843:/checkout/user/register#bottom"
Which has a stray ":" after the hostname.
Since this test passes in the UI, seems like a bug with one of 3 things:
1) the UrlGenerator
2) the CLI test runner
3) the setup in SimpleTest
Comment #12
pwolanin CreditAttribution: pwolanin commentedHere's a patch that fixes both a weakness in the generator code and the CLI test runner, also combines part of dawehner's change in #10 (he had to deliberate debugging fails in there).
The latter fix seems to be enough locally - but I'm running on port 8082. Let's see what pifr thinks. I think this generator fix should also be filed upstream.
Comment #13
dawehnerOpened a fix for that in upstream.
Comment #14
dawehnerhttps://github.com/symfony/symfony/issues/8901
Comment #15
dawehnerWe could just inline that variable assignment
Comment #16
pwolanin CreditAttribution: pwolanin commented@dawehner - why I think that's less readable and doesn't make any performance difference?
In general I think we are avoiding assign + test in Drual code on one line - the standards suggest they should be separate.
https://drupal.org/coding-standards#controlstruct
Comment #17
dawehnerI am wondering whether we should add explicit test coverage for the simpletest runner change.
Comment #18
pwolanin CreditAttribution: pwolanin commented@dawehner - it's a bit hard unless you're running the tests not on port 80?
Comment #19
Fabianx CreditAttribution: Fabianx commentedGenerally looks good; dawehner wanted to use tagged services for this somehow in IRC? And or made the case when the url generat0or is not available (during install) less fatal or something.
Comment #20
dawehnerWell, I think it is fine to place that but it would be cool to try to understand what is going on wrong during the installer. Aren't most normal services available?
Comment #21
dawehner.
Comment #22
pwolanin CreditAttribution: pwolanin commentedregisterTwig() has another check on drupal_installation_attempted(), which is where I borrowed the logic, so somehow seems things are not complete during install
Comment #22.0
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #23
Dries CreditAttribution: Dries commentedWe want to get rid of the double/old routing system. I think I'd love to better understand how this plays into removing the legacy routing system. If we want to eventually remove the url-function, does it make sense to introduce get_url() in Twig? What do we envision the end game to be here, and what would be the function names?
Comment #24
pwolanin CreditAttribution: pwolanin commented@Dries - I think it's still helpful to have something capable of rendering an external URL, but I agree we could make the preferred ones shorter like url and link, and the one for abs hrefs and paths, longer
Comment #26
dawehnerSymonfy fullstack provides url() which works like get_url() in the patch, so given route information, return the URL. I think we should make the behavior similar to fullstack as people will
read the documentation of symfony, see http://symfony.com/doc/current/book/routing.html
Changes:
Comment #27
pwolanin CreditAttribution: pwolanin commentedThere is a little handling in url() for the case where the generator is not initialized - I'm not sure we need that?
If not, perhaps this change:
should be like:
and use the UrlGenerator object directly also.
Comment #29
dawehnerGood idea. This also fixes the test failures ...
Comment #31
dawehner#29: twig-2073811-29.patch queued for re-testing.
Comment #32
pwolanin CreditAttribution: pwolanin commented#29: twig-2073811-29.patch queued for re-testing.
Comment #34
dawehnerThat's it.
Comment #36
pwolanin CreditAttribution: pwolanin commentedThe route name changed.
Comment #37
dawehnerGood catch!
Comment #38
Fabianx CreditAttribution: Fabianx commentedSo Symphony provides url() and path() functions. One of them outputting absolute urls if I see this correctly.
Can we do the same here? I am okay with the legacy function.
Comment #39
dawehnerOpened a followup: #2095201: Decide whether a path() twig function would be useful.
Comment #40
alexpott#36: url_generator-2073811-36.patch queued for re-testing.
Comment #41
alexpottIs this actually necessary? Especially given that https://github.com/symfony/symfony/pull/8903 has been closed with no fix?
Doesn't this change
Fix it?
Setting back to "needs review" to get an answer.
Comment #42
pwolanin CreditAttribution: pwolanin commentedThe change to run-tests.sh should be sufficient at least for this patch (I think - will test)
Comment #43
markhalliwellI understand the need for
url_from_path
. However, we cannot continue simply adding new functions (to Twig) for adding functionality. This should be extending the existing url() function with an option of like {'fromPath': true} or something. This "switch" if you will, would change the logic of what url to return. We are trying to make things simple for front end, not more complicated. 1 function with options that can be extended over time (and listed on one API doc page). To give some perspective, it would be like me creating a whole new {% trans_options %} tag just to handle #2049241: Add support for language options to the Twig {% trans %} tag extension.Why is this function needed? If we're already at the template level, why can't we just create our own
<a/>
tag and use the url() function? Do we need to send links through the theme system? If so, for what is the reasoning behind this, preprocessing?Comment #44
dawehnerI am not sure whether an additional parameter makes it easier as a single method with an additional parameter. At least it is good to know that you agree that the url() handling route_names and route parameters
should be the default, as this is the same in symfony fullstack.
This is for sure not needed but it seems to be more consistent to have support for both.
Comment #45
markhalliwellI am sure.
_from_path
is just a simple backend logic switch, nothing more. There is no need for having an entirely separate function when an if/else condition could be placed in thegenerateUrl
method on the backend to detect when a{'fromPath': true}
optional parameter has been provided.Front-end only needs simple functions (usually). They will likely view the documentation and see that "Oh, I can provide X option to this function to change things. Awesome." What we hate, is when we're given hundreds of different functions that all do the same things, but each doing things just a little different. It makes it difficult to maintain, change and remembering which function does what.
They both end up providing a URL. Where that URL comes from should be determined by optional parameters passed to the function.
Consistent for whom? The biggest pain we have as front-end developers is being given markup that we don't always want. Yes core might provide an
<a/>
tag to wrap this URL by default, but that doesn't mean it should be using a special{{ link() }}
function to do so. It should instead be doing things like:<a href="{{ url('node/' ~ node.nid) }}">{{ node.title }}</a>
in template (or whatever syntax is being proposed here forurl()
).For example, my node.js app doesn't need the entire
<a/>
tag, it just needs the URL. By keeping the<a/>
tag in the template, this lets me (the themer), to strip out just the extra (unneeded) markup right from the template I copied to my theme. I don't have to go hunt for which function to convertlink()
to because I don't want that extra markup. So instead of introducing a whole newlink()
function, instead we should be forcing templates to keep the markup in the templates themselves. This is what would provide real consistency.Please, don't provide themers some magical backend function that spits out (unnecessary and unwanted) markup. Just give us the data. It's our job to determine what needs to be done with it, not the back end.
Comment #46
mortendk CreditAttribution: mortendk commented+ 1000 for giving us the data to work with and not the {{ magic_url_with_all_the_stuff_a_theme_cant_split_out }} it makes everything a pita.
a total seperation of markup & "the machine" (drupal) is the only way to go
This is might what i want today
<a href="node/{{ node.nid }}" title="{{ node.titel}}"> {{ node.titel}} </a>
but in 3 months im gonna work on a project where i only need the
<b class="foo">{{ node.nid }}</b><i>{{node.title}}</i>
cause "something something" happend - we never know what happens in the frontend, just that it will change :)
... pretty pretty please just the data :)
Comment #47
steveoliver CreditAttribution: steveoliver commentedAside from the unadressed items from @alexpott's review in #41,
I think I like
url_from_path
. Here's why.Comment #48
markhalliwellYes, I agree that #41 needs work according to the GH PR.
Regarding
url_from_path
vsurl(..., {'fromPath': 1})
, this isn't about character length (nor would any themer really care about this). This is about conceptually keeping it simple. They both return a URL. Do not provide two different functions that spit out the same type of string. No where else do you see Twig doing this. We provide options if/when we want to alter the output of X function.Comment #49
steveoliver CreditAttribution: steveoliver commentedYeah I guess that does make sense if we don't want people generating urls from paths.
Comment #50
webchickHm. I'm not so sure. #47 rings really true for me as an occasional dabbler in theme/module development. Having to pass that extra ugly param is not only bizarre, but also error-prone (if you forget the closing }, spell it "formPath" instead, etc.)
People are going to simply want to create a link to things, via the path since that's the most natural entry point since they learned in internet 101 that things have URLs, not weird route machine names. :P
I think ideally, url() would be changed so that it just inspects the argument being passed in and does the right thing without any other hairy parameter passing. Maybe we could actually enforce the routing naming convention we've established and then introduce logic like if the value starts with "$module." it's a route name, but otherwise it's a path? Not sure.
Comment #51
pwolanin CreditAttribution: pwolanin commentedIndeed - we sure be discouraging the use of system paths (hence the longer function name for the discouraged case) and the arguments to the 2 functions are different, so I don't see how we can have an option that makes sense.
In template files you should generally be using routes - unless it's for a site-specific theme that has some hard-coded crap.
Also, I'd still think the link() function makes sense because it can do things (at least currently like adding the active class) that I'm not sure it's possible to do otherwise in the template.
Comment #52
markhalliwellThis is completely false. Twig parses syntax automatically and fatal errors if it detects invalid syntax (ie: missing }). This is standard Twig behavior when it tries to parse a template.
We can instead do something like
{{ url('...', from_path = true) }}
(@see: http://twig.sensiolabs.org/doc/functions/include.html) This would allow us to also explicitly detect when an invalid named option is used and again throw a fatal error.Oh please god, no. We do not want to introduce yet another Drupalism like we encountered with format_string variables (!@%) in #1927584: Add support for the Twig {% trans %} tag extension.
Which is exactly the point I am trying to make! If we provide two functions, people will see those two functions in a list. Instead of just seeing one function that they would have to drill down into the docs to figure out the optional parameters you can pass. There is absolutely no need for creating a second function for such a simple logical switch on the backend.
Valid point. Well, if we are going to do this, then let's at least call it and map it to the l() function (which is actually what adds that "active" class). I'm still really hesitant to introduce this though (for reasons mentioned above).
Comment #53
dawehnerI am glad that other people ignored this comment, because seriously, the data is not the NID. This bypasses every system we have in core
and causes major pains, so let's better add nice to use helper methods so people can use them.
Regarding the url(with some parameter) vs. url_from_path I would even recommend to rename the url() method in core itself to avoid confusing. Using url with parameters is the recommended way everywhere, so let's try to help people to not fall into the legacy system.
Comment #54
markhalliwell@dawehner, I'm a little confused. Are you suggesting we change the core method/function url() name or the Twig url() function name? If so, I'd be fine with the latter. Maybe to route()? Idk, regardless of the name change though, a Twig function should not be "cloned" to handle a different set of "optional" configuration. So I'm think you're talking about something like this, yes?
{{ url('node/' ~ node.nid) }}
->http://example.com/drupal_install/my-awesome-path
and if we wanted to pass the path (after the route has determined where it's gone):
{{ url('my-awesome-path', from_path = true) }}
->http://example.com/drupal_install/my-awesome-path
Comment #55
thedavidmeister CreditAttribution: thedavidmeister commented@Mark Carver, the actual *blocker* to putting
<a>
directly in templates is the active class handling, that is currently impossible to simulate in a template. #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links is currently deliberating over whether to move .active to js (which would fix that), but there's been pushback on that idea for various reasons (I'm personally still keen to get .active off the server completely).Beyond active class handling, the benefit of having a link() function *is* largely convenience, this is true, but in context of what I've written below, be careful not to understate how much is automated (in a good way) by our current link generation functionality. This comes up time and time again, but I remain unconvinced that it's usually better (and therefore core "should" do it) to replicate all of the below that is provided by '#type' => 'link' (or equivalent link() function) in a template with "raw"
<a>
tags than it would be to use a link() function and implement a preprocess for your template or hook_link_alter.Regardless, this conversation seems to be drifting towards a focus on "trees", maybe it's worth taking a bit of a step back here and getting some more contextual "forest" in the discussion.
What we want:
What we have outside Twig, in '#type' => 'link':
hreflang
attribute if we know the language of our URL.active
class based on the current URLThis was discussed a fair bit in #2047619: Add a link generator service for route-based links when the linkGenerator was first added, but we don't want the theme system to have to have "duplicate" functionality to handle paths and routes separately. One reason for this is that it would immediately lead to duplication of all templates that want to incorporate links - theme_links_path/theme_links_route anyone? "Just one more function" would become "just 50 more templates", or something like that. I'm entirely unconvinced by #47, to me it seems to go against the grain of the discussion and conclusions we came to when the linkGenerator was originally being developed and is philosophically counter to efforts like #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays.
Any url/link generating functionality we encourage developers working with the theme system to use should work equally well with routes, paths and also external/absolute URLs.
I think it makes complete sense to allow parameters to be passed to url() in Twig. Maybe it seems like total overkill if the *only* parameter is an is_route style flag (I'm assuming the default behaviour is for url() to assume data is a route, so 90% of the time the parameter is unnecessary anyway), but if we support query parameters and language handling too then it's just one of multiple options that url() accepts. What might be an option is to make parameters for a Twig url() look more like '#type' => 'link' so that, rather than a flag, you pass a path or route explicitly.
On that note, I'm surprised that, in the discussion of whether to go with
url('just a string')
orurl(parameters...)
, language and query handling hasn't come up. I'd like to hear from proponents of the former what the plan there is?Comment #56
thedavidmeister CreditAttribution: thedavidmeister commentedOh, btw, the reason I added "Needs issue summary update" is because the summary only mentions routes, not absolute/external URLs or paths, but it's something that is discussed in the comments.
Comment #57
dawehnerI totally respect how much you care about themers/twig-users but I disagree that we treat them as stupid persons. They should
just get the same power, interface to drupal as other people, as it helps them if they understand the concept behind functionality.
I still don't get why you try to insist on this statement. A function should do a single thing, so that with two different functions you just know which one is doing which. Otherwise you end up again in a mess like drupal_add_js() was before. As said before, let each function be KISS.
Especially point three is really important. In general I simply can't see how an additional parameter somewhere is easier to grok, especially for existing twig people (which probably come from symfony, which do have the same function (url()) already).
I don't know why you actually have to care about this. Here is the interface what is behind url() so this is everything supported without any additional effort.
Given that, here is a patch which fixes the good catch by alex in #41.
Comment #58
thedavidmeister CreditAttribution: thedavidmeister commentedI've explained that duplicate url()/link() functions leads to duplicated templates where themers may want to use a template for routes, paths or external/absolute URLs in different situations and they put a url()/link() function inside the template.
I don't "get" what's to "get" about that being undesirable.
I think of this more as BC in url() that is intended to be removed in the future rather than a deliberate move to a situation where we actively develop two parallel APIs in a single function.
Comment #59
Fabianx CreditAttribution: Fabianx commentedSo chiming in here with our goals:
We want to add a url generator extension to make creating urls and paths from twig templates possible. The link part is out of scope.
Given the theme system maintainers spent around 6 months on re-factoring links already, please put that to another issue.
a) We want to be Symphony compatible: http://symfony.com/doc/current/book/routing.html#generating-urls-from-a-...
b) We want to be Twig syntaxy compatible: http://twig.sensiolabs.org/doc/functions/include.html
c) We want to make the switch as simple as possible
a) We reach that by including two functions url() and path(). Anything else is a Drupalism and we should avoid those as much as possible.
Therefore => path() (relative URLs) and url() (absolute URLs).
b) Twig has powers to have very efficient syntax, therefore it is possible to create an url() or a path() with a very nice syntax, which semantically is the same as the url_from_path or path_from_path (WTF!) functions, but nicer to write. Also compatible with the twig mindset and any other options we add as parameters:
Given that symphony does only support the options and those are optional this will remain BC compatible to Symphony for a long time to go, which is good!
c) Make it as easy as possible to convert:
{{ url('node/1') }} -> {{ url('node/1', is_path = true) }}
is simple enough to remember and write.
d) Make it better?
I would even go so far and support that we only support certain needed legacy URL options and not the whole url() options support in Twig templates:
And we support those as:
BUT we hardcode them to those that themers really need instead of supporting all of them.
---
Edit: Regarding the scope of this issue: Links are complicated. A link function could also use an OOP style approach - or just return variables or even just a link
render array.
Example:
But that is another discussion that needs to be done in a wider scope and also discuss how to support render arrays existing and not introduced here in the change of url() in twig to support routes.
Comment #60
markhalliwellAgreed. Since no one seems willing, I'll create the Twig extensions.
Comment #61
dawehnerI am sorry but I disagree with that. People should be encouraged to use routes, so it is not a matter of making things easy to convert.
Comment #62
jibranSending #57 to test.
Comment #63
markhalliwellHmm, I guess I forgot to put this in my comments above (if TL;DR, skip to next section for possible solution):
I do agree with @webchick's cooment in #50:
I am not opposed to routes whatsoever. I know they're the "future" and "here to stay". The reason I am adamant about all of this though is the tag I just added: FX (front end experience). We are the ones that ultimately will have to use this on a daily basis (and will be upgrading existing themes).
Not only are we changing the way existing Drupal themers use url(), but we're basically saying: "we don't care about your PITA upgrades". You'll make them want to use the "deprecated" function because that's what they know. So you'll start seeing themes with
url_from_path()
everywhere and in turn alienate the forward moving functionurl()
from being used properly. I think it's more important to ease them in and give them an opportunity to learn the right way (by enforcing ideology through documentation) instead of drastically throwing them to the wolves and causing things to just not work right (in their eyes).Also let me clarify that there are levels of front end experience. Yes, I, along with a handful of others are probably more adapt to understanding the backend of how things work and why routes are important.
However (and a huge one at that), most themers will have no clue what a route is, nor have an easy way to figure out what they are. There is no 1:1 correlation between a route and url path (ie:
user.register
!==user/register
). Most will (literally, I have seen this time and again) attempt this first:<?php print url('..'); ?>
, which of course they think should work for:{{ url('..') }}
It isn't until probably after a couple hours (give or take) of digging and trying to figure out what a "route" is, that they finally realize that this is yet another back-end "this is the way it has to be, because this is now how the back-end determines url structure" implementation. I think it's important to let them learn these routes and be introduced to this concept overtime so they can memorize them and work with them as it's truly needed (believe me, they're smart enough to know when they need to start using them). Just don't making upgrading existing themes (initially) a PITA please.
---------
After talking with @Fabianx in irc, perhaps we really should do something like what @webchick suggests in #50:
I still don't like the
$module
prefix idea (it's just another Drupalism). Maybe we could instead implement a single{{ url() }}
function that takes any number of arguments (because they differ between generateFromRoute and generateFromPath and use a try/catch:Comment #64
markhalliwellComment #65
markhalliwellProviding patch
Comment #66
dawehnerYou know the problem is that thrown exceptions are totally hidden, so people will never know when they did something wrong here, like wrong naming of some route parameter etc.
Comment #67
pwolanin CreditAttribution: pwolanin commentedHaving to catch exceptions to handle a href seems really ugly.
What about passing in named arguments, in analogy to #type => link? http://twig.sensiolabs.org/doc/templates.html#named-arguments
e.g:
is the same as:
But you can use the named href to get a path-based generator:
Comment #68
markhalliwellNot really. It will still throw an exception at the path generation because "user.registar" isn't a valid path either.
This has nothing to do with links anymore, this is just the URL. The exception will only fire if no matching route is found. This also means we don't really have to worry about paths because they're deprecated anyway (ie: they shouldn't be using it).
Comment #69
pwolanin CreditAttribution: pwolanin commentedNo, I don't think this is viable as is.
There is no validation on paths - it will just give you /user.register
Also, I think we must include a link generator - we can put that in another issue, but that's not what I mean. Rather I meant an explicit switching based on which arguments are used.
Comment #70
markhalliwellAfter having a huge IRC debate about this and being told that "paths are WRONG and shouldn't be used", we're going to go with routes and remove (the already deprecated) paths. The biggest issue I ever really had was: providing two different ways to generate a URL. If we eliminate paths and force routes, this will help the "you can do it X number of ways" and the headaches to follow. (edit: yes, I know I said upgrading will be a PITA if we didn't have path support, but having two different ways to do this will ultimately be worse.)
We have also determined that if we are indeed going to do this, then we should probably follow standard Symfony syntax/functionality: https://github.com/symfony/TwigBridge/blob/master/Extension/RoutingExten...
In an effort of compromise, I think we have agreed that a needed helper function (in a separate issue) should be added to determine a list of available routes:
{{ dump(routes()) }}
or{{ dump(routes('^user')) }}
(for routes that start with user)This will allow themers (not familiar with routes) find the ones they might need.
Assigning to @pwolanin for a bit to work on it as I am obligated elsewhere atm.
Comment #71
pwolanin CreditAttribution: pwolanin commentedLooking at the TwigBridge, I don't think we want to use it directly (if nothing else, Drupal's version of the generator doesn't even support scheme-relative URLs!).
However, matching the basic usage and behavior of of path() and url() seems like a win
Comment #72
thedavidmeister CreditAttribution: thedavidmeister commentedLast time I checked, the linkGenerator for routes could not handle absolute/external URLs. Is this still the case?
Comment #73
dawehnerI guess it would make sense to move the l() code into a new method on the link generator, so you have kind of a similar interface as on the url generator.
Comment #74
pwolanin CreditAttribution: pwolanin commentedYou can certainly generate absolute links via the UrlGenerator and LinkGenerator.
Here's a new version making it work mostly in parallel to the TwigBridge extension.
Comment #75
dawehnerThis is looking quite great.
I do agree that we should try to enforce that people don't use urls, and just route names,
though this requires us to provide some tools to help with routes.
Comment #76
dawehner.
Comment #77
webchickI'd like to confirm this approach is ok with the front-enders.
I don't have a problem with this per se, since it matches what Symfony/Twig devs will be expecting, although it does seem like it's defining url() to be completely differently than the url() function in Drupal (I think "path()" is actually Drupal's "url()"?), and it seems like a total WTF to have two functions called the same thing that do wildly different things. Or am I wrong about that?
Comment #77.0
webchickUpdated issue summary.
Comment #77.1
markhalliwellUpdated issue summary.
Comment #78
markhalliwellYes, after much debate. We've agreed that, in an effort to keep things consistent between the front-end/back-end and Symfony, this is fine. The "routes ship" has long since sailed.
@webchick, you are correct. There is no correlation between D7's
<?php print url(); ?>
and D8's{{ url() }}
. However, Twig has never shipped with a url() or path() function out of the box. The current url() Twig function we have in core was always meant to be temporary until routes became the de facto. The reason this sparked this whole debate was whether or not we should leave in this legacy "path" code or not.Ultimately, it's been decided that there is no sense in providing backwards thinking and confusing people. Regardless of whether or not we provide an entirely separate function (
{{ url_from_path('user/register') }}
) or argument based option ({{ url('user/register', from_path = true) }}
, will will in turn alienate the forward moving (route based) functions. We'd start seeing these deprecated functions/arguments everywhere (in custom/contrib), instead of seeing clean and concise forward moving code. We do not want to introduce yet another "Drupal theming: you can do it X number of ways" headache.D8 no longer really has the concepts of "paths" anymore anyway (the method that does exist in the urlGenerator is deprecated and will/should be removed). The compromise that was reached for front-end, is that we provide the ability to discover routes easily through helper functions.
I've updated the issue summary and created these follow-up issues:
#2106811: Create route discoverability (back-end)
#2106797: Create a Twig function for route discoverability (front-end)
#2106859: Add a link generator Twig extension
Comment #79
Fabianx CreditAttribution: Fabianx commentedI fully agree with #78. RTBC +1
Comment #80
steveoliver CreditAttribution: steveoliver commentedThanks, guys.
i. I accept as reasonable the argument made for allowing "paths" (href's) to only be supported in templates without passing them through url() or path().
ii. Now I see a big WTF when you look at route parameters vs. 'options'. Why not put route parameters in options? It'd avoid that ugly ass empty second arg and keep the api. Also, in options we can better document where query params should go (I don't know, is that in parameters or options?)
iii. Should this redefinition of 'url' to mean absolute href, and 'path' to mean relative href, also apply to Drupal PHP in addition to Drupal Twig?
iv. As the patch stands, I have these nitpicks (points 6.) and 8.) may not be relevant if we change the signature of url() and path() to add params to options):
This doesn't seem like quite the right title.
Maybe "Sets generator services for the Drupal core Twig extension." ?
"Returns a list of functions made available to Drupal Twig templates."
s/function/functions/.
Looks like an extra new line here.
typehint 'string' $name.
An associative array of route parameter names and values?
Needs an additional newline and 'string' typehint for $name.
An associative array of route parameter names and values?
Move { up on function line.
What's $link_generator doing in TestTwigUrlGenerator() ?
This shouldn't be here. See comment See #14.
Comment #81
webchickI'm going to try one more plea for sanity in this issue, and then I probably need to unfollow it for my own mental health. :(
That's code that's written by every. single. web designer. within their first about 8 minutes of being a web designer. Don't know what goes in "foo/bar" for this link? Navigate to the page you're trying to link to, copy the end of the URL, paste it. You're done. Anyone who's ever used the Internet has used URLs. They're a universal concept.
The big stated win of Twig in the first place is that it's accessible to non-developers. You don't need to know PHP in order to write a theme for Drupal anymore. Huzzah! All the most common dynamic properties are provided for you, and you simply print them out in your template. Heck, you can even use Dreamweaver or whatever standard tools to do it. Twig has finally made Drupal theming accessible to the masses who understand HTML and CSS but not necessarily PHP. That was the entire point.
So then, why on earth are we thinking it's a good idea to require that very same target audience to write code like this:
Writing this line of code requires:
1) To understand what a "route" is, which comes up not once in any HTML/CSS books.
2) To understand what a Drupal "module" is, which comes up not once in any HTML/CSS books.
3) To understand what "YAML" is, which comes up not once in any HTML/CSS books.
4) To know how to dig around in various module.routing.yml files to find whatever the machine name for a desired route is.
5) To know what a "function" and "function arguments" are and how to use them.
6) To master the specific complex and complicated syntax for argument passing for this url() function, which probably involves getting it wrong 2-3 times and ending up with fatal errors in your face and having to debug them.
7) Probably other stuff I'm forgetting.
... none of which is required to simply pass a path into an anchor tag.
Is having two functions available (url() and url_from_path()) to return a URL adding extra complexity to the theming system? Yes. Is fighting complexity generally good? Yes.
But I literally cannot understand how the additional complexity of an extra, optional function is a worthwhile trade-off for the horrific mass of complexity that's required to do this task which could not possibly be more common in web development. Seriously. Help me understand how we are not destroying all benefits we aimed to get with the switch to Twig by pushing route machine names down to the theme level?
Comment #82
steveoliver CreditAttribution: steveoliver commentedAfter all this, I propose:
1. url() from D7 becomes url_from_path() in D8, usage is the same as D7's url function. @deprecated. @see url().
2. url() in D8 works with routes only. We need to reconcile our current options array with route parameters - I think there is overlap.
3. no path() function in Twig.
Comment #83
dead_armMuch of what has been said is about adding a level of complexity for themers, but this is my input as a non-PHP daily HTML and CSS person:
When I need to add a link that is within the site, it is pretty rare. When writing a link with actual HTML <a> tags it is most common for it to be for a link that doesn't really change, and is usually absolute, like a company's twitter url in the site footer, and I would usually do that using the Full html format in a block.
If I need to add a link to somewhere in the site, I know better not to hardcode the path. Doing it the "right" way is out of my comfort zone, meaning that I don't have it memorized, so I have to look up documentation, search through issues, and/or ask a developer on my team to help. I spend this extra time on it because I want it to be right, so that I can theme against it one time. In the sites I work on, pages move around all the time, so I can never hard code a path to something in the site, because it will inevitably be changed. When it happens, then my work doesn't apply anymore which is the worst possible and annoying thing that happens to themers IMO.
I actually like the idea of having a machine name that won't change, because done correctly I can theme against it and know that wherever it gets moved my styling will still apply. The one given is that how to implement a link properly is accessible and documented, and with Twig it is, so I really don't have a concern about that. Themers that value their time and effort will appreciate being able to apply against non-brittle markup.
Comment #84
webchick#82.1 would assuage my concerns. I actually am not opposed to making url() and path() match what Symfony does, but if we do it in Twig we need to do it in Drupal proper, too, or that will be tremendous WTF.
Comment #85
steveoliver CreditAttribution: steveoliver commentedThen likewise a path() in PHP to return relative paths, since url() will always be returning absolute hrefs. +1.
Comment #86
steveoliver CreditAttribution: steveoliver commentedThe url_from_path() function wouldn't totally make sense considering the use case of generating relative hrefs with the function. url would mean absolute.
How to create a path ...from a path? path_from_path() Seriously. At least conceptually, if not by name.
Consider:
// Generate web addresses for a given route:
url()
path()
// Generate web addresses for a given path:
// @deprecated
// @see url() and path().
url_from_path()
path_from_path()
Comment #87
markhalliwellAnd we're back to trying to introduce a second function that returns the same thing..... NO! *FACEPALM*
What it boils down to is this (edit: sorry for yet another long comment):
-------
@webchick, I'm sorry. I'm almost right there with you :( I would like to point out though:
<a href="{{ path('module.foo', {}, {'foo': 'bar'}) }}">blarg</a>
Writing this line of code requires:
1) To understand what a "Twig" is, which comes up not once in any HTML/CSS books.
Most novice themers (regardless of CMS), usually end up just manual inserting the URLs to begin with anyway. They wouldn't know about about any functions because they don't really know the language the template is written in (we have had this issue in PHPTemplate as well, Twig is not special). It isn't until after usually:
1) The site their working on becomes bigger - and
2) The templates they created start breaking when copying them to different sites
3) They provide a patch and are yelled at experienced contributors that it's "WRONG" ...
... that they hunker down and start reading documentation or asking questions about the CMS and how to generate internal URLs to make themes site generic and not site specific. However, this is simply a learning curve of any new themer on any CMS.
Justifying that these Twig functions, which are advanced use cases anyway, should be "easy to use" for novice themers.... that's just silly, they wouldn't even know they exists. When they finally read the documentation for it, they'll understand it because "it" is: "Twig functions for returning a URL of an internal Drupal route."
------
Below is just my observation and agreement with @dead_arm in #83, which btw she is 100% correct:
Exactly! We're all griping about functions that will be used in very advanced use cases. When we want (and what these functions allow) is to take complete control of that said template and do whatever the hell we want with it. (ie: NOT using provided variables, which BTW.... does contain the links that we're probably overriding to begin with. These URLs will likely also be completely unrelated to the template at hand anyway.)
LOL :D this is SOOO TRUE! However, I would also like to point out that we do tend to learn and retain that information over time though. This isn't our first Drupal rodeo. We know how to google and we know how to ask questions. Plus given all the documentation, blogs, pod/blog-casts.... this really isn't something that is "hard" to learn. It's readily available for those who seek it :)
If we provide two different ways to return the "url" (url() vs. url_from_path()), this will making it "right" an absolute nightmare to maintain. Especially if you're not the only themer on a project! One uses url() and others url_from_path(). I am really baffled at why this is so hard to get across.... so all caps:
HAVING TWO FUNCTIONS THAT RETURN THE SAME TYPE OF STRING DOES NOT PROVIDE CONSISTENT SYNTAX FOR THE FRONT-END
CORRECT:
These are two completely different string outputs for the same route.
url('user.register') => http://example.org/user/register
!==
path('user.register') => /user/register
INCORRECT (proposed):
Regardless of either "absolute" or "relative" variants, these two proposed functions output identical strings (from path or route):
AMEN!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
Comment #88
markhalliwellActually, I'm assigning to @Dries for review. We've all said our piece and played PONG. We need a final decision on this.
Comment #89
dawehnerTherefore though I think someone should write a proper issue summary.
Comment #90
dawehner.
Comment #91
thedavidmeister CreditAttribution: thedavidmeister commentedI can accept a function that deals with routes and not paths, or a function that can handle routes and paths, I don't think I could be happy with 2 functions that do the same thing because it leads to duplicate templates.
As soon as core wants to use one of the 2 functions in a template, we'd have to provide another identical template with the other function, if we do indeed support the second function. That's crazy.
@webchick, I agree about wanting
<a>
tags to "just work" as much as possible. Can you please throw some of your reasoning over at #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links because the "opt in server-side" resolution currently being proposed basically forces "links done right" to be beyond the scope of all novice themers, which is sad IMO when we do have some js alternative options there.Comment #91.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #92
Fabianx CreditAttribution: Fabianx commentedThere is another use case we did not think about:
* Non CSS-Images
D7:
D8:
???
Maybe a compromise is to make base_path available within Twig?
That would allow the themer to write and learn:
but it would also allow the novice themer to do:
or
We need to support some kind of base_path function anyway for images as they are no routes obviously and this is better than a full fledged url or path function.
It is also simpler.
I will ping @webchick with this alternate idea. I never liked to write url() in templates anyway in D7 :-D.
@dawehner:
What happens if a route is not found? i.e. what error messages do path() and url() return?
Comment #92.0
Fabianx CreditAttribution: Fabianx commentedUpdate issue summary
Comment #93
webchick{{ base_path }} is a great idea, and would very much help assuage my concerns! Thanks, Fabianx.
Comment #94
dawehnerRight, sadly this bypasses url aliases, language prefixes/query arguments and what else.
Comment #95
xjmThanks @dawehner -- Agreed that we definitely need an issue summary update here to cover the different perspectives for this issue. It would be especially valuable to have that issue summary update before we walk through this issue with Dries. @Mark Carver, can you help get that summary update here so that we can take Dries through the issue? Assigning back to you for the moment with that hope; please reassign to Dries when you think everything is clear. Thanks!
Comment #96
Wim Leers#92: no, using
url()
in D7 is completely wrong. You should usefile_create_url()
for files, always. Otherwise, the file URL cannot be rewritten to be served from a CDN.Assuming
base_path
is the root for all files is precisely the thing that was fixed in D7.Comment #97
xjmEr like that. :)
Comment #98
Fabianx CreditAttribution: Fabianx commented#96: That is true and you are right url() is wrong, file_create_url is right here.
I think however the novice themer might neither have a multilingual mega site nor a need to have this theme only file on the CDN. He might most of the time just write /my-file anyway unless developing in a sub directory, where using base_path() is quite natural.
Having a base_path() is however still useful information on its own - even if it can be mis-used in wrong ways.
And both cases of inserting images and links in the theme are still the exception as we have a CMS, which manages this content usually ...
But we kind of obviously also might need a file_create_url or whatever called function in Twig or the easier ability to create render arrays on the fly - after all I also want to apply an image style and my theme should override my image output and ... ;-).
I do propose though that we continue _this_ discussion in further follow-up issues.
#95: I already updated the issue summary to the best of my ability including a pro / con section, but left the tag for others to add their POV, too.
Comment #99
Wim Leers#98: agreed — wise words :)
Comment #100
effulgentsia CreditAttribution: effulgentsia commentedI'm +1 to this. Looks like that issue is resolved, and no twig files currently call url() anyway, so regardless what else we do here, let's at least do this much.
As to the rest of the patch,
So why do we need this patch then? If it's a seldom activity, aren't PHP preprocess functions sufficient? Can we get some good use cases posted in order to evaluate that?
Per #98, we don't have that, and this patch doesn't give it to us. It makes creating URLs to Drupal routes consistent, and I see some value in that, but then creating URLs to uploaded files, URLs to code files (e.g., CSS), and URLs to external resources, all follow different patterns. I'm not sure that naming the route-based one
url()
, and then needing alternate names for the others is the best DX. And I'm especially concerned about doing that while we still have Drupal's legacy url() PHP function meaning something completely different.I get the desire to want consistency with Symfony (http://symfony.com/doc/current/book/routing.html#generating-urls-from-a-...), but Drupal is different than Symfony: Symfony is primarily about routes, whereas Drupal is about routes and uploaded files and modules that can be relocated to other directories and other stuff. So to evaluate this holistically, perhaps we should come up with good use cases for all of these different things that themers could want to link to, estimate how common/rare each category is, and then come up with what syntax we'd want for each one?
Comment #100.0
effulgentsia CreditAttribution: effulgentsia commentedUpdate comment to #92
Comment #100.1
markhalliwellUpdated issue summary.
Comment #100.2
markhalliwellUpdated issue summary.
Comment #101
markhalliwellI've updated the issue summary. But after this though, I'm honestly done (edit: debating this issue to death). I feel that this issue keeps becoming scope creeped with "all possible uses cases". God forbid we create separate issues to address things like adding a
{{ file() }}
function or a global{{ base_path }}
varible to handle these special use cases. This issue has always been about routes to pages.Comment #102
effulgentsia CreditAttribution: effulgentsia commentedThanks for the updated issue summary, and I apologize if #100 was too abrupt or dismissive of the work that went into this. To restate my concern, I searched D7 core's *.tpl.php for an occurrence to url() or l() and found none. Because of that, and since I'm not a themer, I'm unclear as to what kinds of use cases come up. But from what I can tell so far, we'll need to support at least 3 kinds of URLs:
- to routes
- to entity operations (which per #1970360: Entities should define URI templates and standard links will likely need to have their own syntax that isn't route-based)
- to files (if we get #1308152: Add stream wrappers to access extension files done, we'll at least be able to unify the syntax for uploaded files and shipped files)
While I think it's fine for each of these to be implemented in individual issues, I think it would be good for us to agree on what we'd want the syntax of each of these to be prior to committing the first implementation, since at least for me, I find it hard to evaluate whether
url()
is a good name for a function that only handles a subset of Drupal's URLs. On the one hand, that's the name already in use by Symfony, which is definitely a strong point in its favor. On the other hand, it collides with Drupal's legacy url() PHP function, and I'm not yet clear on whether we'll remove that in D8, and if not, that's a strong point against grabbing that name in Twig. So, I think it would be good to have a plan in place for the syntax we want for generating these different kinds of URLs, and ideally, a syntax that's consistent in Twig and PHP.Comment #103
effulgentsia CreditAttribution: effulgentsia commentedAlso, removing the "Needs issue summary" tag. There might be more we want to add to the summary at some point, but for now, I think it summarizes the issue well.
Comment #104
steveoliver CreditAttribution: steveoliver commentedThank you, effulgentsia.
In 100% agreement with #102 the holistic appraisal and conversation we need to have about all these related topics.
I agree we need to back out of Twig and assess where we are with everything related to urls, paths, routes, parameters, etc., especially around the transition from old to new thinking, and make the hard decision(s) in the largest scope we can, then come back to this as an implementation of the larger discussion.
Since this issue has always been concerned with Twig, I'd like to set it postponed (also, I don't see it as major) and find or create a higher-level [meta] to bring these discussions to. If anyone knows of an appropriate issue, please chime in -- I'll be looking for related issue(s), so those DX tags should probably help :)
Everyone++
Comment #105
markhalliwell@effulgentsia, no you're fine. Sorry for my abruptness too.. I'm just sooo done with this issue lol I'll answer your questions though:
Because it's as simple as
{{ url('user.register') }}
in a Twig template. We're trying to eliminate the need to always go to PHP for things in Twig. To do the equivalent in PHP, you'd have to do something like the follow. Which since I'm writing it here, should give y'all a good idea of that even I myself have issues with knowing how to do some of this OOP based programming off the bat. Ultimately, this would frustrate the hell out of us themers because we'd then also have to how Drupal works internally via PHP (not just Twig):This is a lot more code to write, especially when all you want is just a quick and easy way to get the correct URL for a route (since it is rarely used).
Actually the only thing that really might need a separate Twig function is the
module://, theme:// and profile://
bit:{{ stream('theme://bartik/css/styles.css') }}
. I actually like this though, because this relates to a canonical file system path. Routes do not, they're dynamic and change based on internal variables.And if I'm not mistaken, entities have their magic "get" methods, yes? So couldn't we just do:
{{ node.uri }}
and{{ file.uri }}
? At render time, it would call that property (or the magic "get" method, if it doesn't exist, to generate it).Comment #106
markhalliwellUgh, fine. Let's make this a lot more complicated than it really has to be. I'm done with this issue. Un-followed.
Comment #107
dawehnerWell, the reason is probably that drupal is way too generic in order to hardcode any URL somewhere. Symfony itself does way more with custom templates than drupal does.
Nevertheless we still at least provide tools to custom theme developers.
Comment #108
pwolanin CreditAttribution: pwolanin commentedWe shoudl make a follow-up to #1970360: Entities should define URI templates and standard links to either list the routes NOT the paths in the annotation OR derive the routes form the annontaiton. The current state of core entity links is insanely inconsistent with everything else we are doing.
Comment #109
steveoliver CreditAttribution: steveoliver commentedThanks, pwolanin - this lead me to an issue I think is concerned with what I was talking about in #104:
#2010184: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method
Comment #110
steveoliver CreditAttribution: steveoliver commentedD'oh! Pasted the wrong issue in re: #109. Here's what I was interested in, when thinking about "links" in different ways:
#1970360: Entities should define URI templates and standard links
Related:
#2106859: Add a link generator Twig extension
Comment #110.0
steveoliver CreditAttribution: steveoliver commentedUpdated issue summary.
Comment #110.1
markhalliwellUpdated issue summary.
Comment #111
steveoliver CreditAttribution: steveoliver commentedOK, I've dug around in other issues trying to get a full idea of where we're at with everything url/path related. Since I don't want to hold this issue up on other uncertainties such as how we'll generate paths to entity operations or support file/stream URLs, I have these thoughts:
Setting to Needs review for others to chime in.
Comment #112
dawehnerI just wanted to ensure that this functionality can be implemented in contrib, so the result worked pretty
good (see https://drupal.org/sandbox/dereine/2118753) beside of #2118763: You cannot implement a ServiceModifier only
Comment #113
thedavidmeister CreditAttribution: thedavidmeister commented#111 seems to make sense to me.
Comment #113.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #114
dawehnerSo anyone here who would like to get this patch in, or should we just let people coming from symfony be confused?
Comment #115
steveoliver CreditAttribution: steveoliver commentedMy comment #111 is about as conservative as it gets from the Drupal side. It means leaving url() alone in Twig, keeping it in sync with Drupal, using it for relative paths and not implementing {{ path() }} as Symfony has. Re: point 1:
Since then I've come to think like @dawehner that the benefit of {{ url() }} and {{ path() }} to Symfony Twig developers may be greater than the confusion to those familiar with Drupal's url()/$path. I actually do think the PHP/Twig domains are separated enough to consider each their own top level namespace.
Considering the concerns from effulgentsia and others including myself about urls and paths to other resources, I wonder: What could be the problem for those resources if we implemented url() and path() as Symfony has for Twig?
1. We need to create urls to Drupal entity operations -- Do we, really? Will we ever be doing that from within Twig? I expect PHP to be generating these links.
2. We need to create urls to files. -- Won't we have file paths generated from PHP just about ...always?
3. What else... ?
All this to say: At this point I don't see a problem with implementing {{ url() }} and {{ path() }} in Twig for parity with Symfony. Also, I believe {{ url() }} should give us our base path.
Comment #116
dawehnerShould this issue just be closed and if someone needs this feature fallback to https://drupal.org/sandbox/dereine/2118753 ?
We seems to be have different assumptions about what Drupal 8 is.
Comment #117
joelpittetre: #92 Created an issue to get those paths in twig: #2168231: Twig Functions needed in templates
Comment #118
joelpittetFrom a front-end perspective... I know how routes work, I know their benefits though I may not use them all the time, I do expect them to be easy to put in to generate a URL. But also, the first thing I ponder is "how do I write a URL that *may* be put into a subdirectory/subsite in a different environment" at a minimum to play nice.
Most likely thing I'd do first looks like this:
And if I needed query params or some variation it would be like:
Now if I did want to use routes, I'd expect them to easier to write than paths in some way to encourage their use, however mildly, over paths.
I'd prefer if the second argument took in all the options needed.
FWIW
Comment #119
dawehnerAs long twig templates coming from symfony still works, which support user/path with route names + parameters I am personally fine with everything,
though even this simple requirement was discussed quite a lot before :(
Comment #120
joelpittetSo to bump this issue: url_from_path() for paths, url() for routes is what I'm getting at in #118.
Comment #121
jenlamptonI think providing two functions that do the same thing (generate a url) is going to make people grumpy.
I think the url() function should be improved to support routes being passed in as the first argument, just as it currently supports aliases as well as system paths.
If the routes are read in from yaml files and cached somewhere, we should just compare the first arg to the contents of that cache to determine if it's a route or not.
Comment #122
joelpittetAs it stands in drupal the url() function still just takes paths (can't do magic both). And there is a Drupal::url() that takes routes.
It seems paths are getting phased out(as much as possible). Kinda the reason I proposed that url_from_path() function was to give access to the legacy path method that everybody is used to but promote and push people into using routes.
Comment #123
joelpittetI do share the concern. I would really prefer not to overload the url() twig function with too many responsibilities and would rather separate the concerns into different functions. Right now Twig url() = PHP url(). Which is path based and being used in core in a few twig templates. Maybe we just leave that as is and add
route_url()
, map that to Drupal::url(). That way there is a clear naming and separation, no extra args passed in for the simplest of calls.Bring on the comments: "routes rule, paths drool" I can take it!
Comment #124
dawehner/me sighs
Comment #125
joelpittet@dawehner 's sighs, take a step back. Who's going to put URLs in templates? I'd hope you come to the same conclusion as me: nobody and if they do, they'll likely be to facebook or twitter or hard coded theme asset. Most links will be fed by the menu system, in the body field or #type=>link.
When themers use url('path/here'). They are saying: "I want that specific page and I want the system to prefix it with the site ROOT and maybe a language prefix".
Now I'm not saying there is no use-case for routes in templates, far from it! Just from what I figure that is not the main use-case for generated URLs in a template.
Maybe you've got a counter point? Are URLs from paths going to be actually deprecated because they are so bad that serve no use in the future?
Comment #126
dawehnerWell, my total point is the recognizability of people using twig outside of the drupal world. All symfony fullstack templates for example use url with route names, so does a lot of the existing documentation.
This reminds me of someone shipping of the island using airraft carrier which is quite good protected.
One point you can add is the problem of integrating existing symfony Routing based bundles/extensions if our url/path methods work totally different. You pretty much can't reuse existing templates but you have to override
all templates for it.
On the other hand I don't think that we don't have any usecase for hardcoded paths, though I am a huge fun to teach people to do things right, because on the longrun everyone will profit. Hint: You maybe write code once, though maintaining it is the hard work.
Nevertheless the contrib module linked above will at least implement a mechanism to be able to reuse existing twig templates.
Comment #127
thedavidmeister CreditAttribution: thedavidmeister commentedYou want to be able to put routes in templates.
Routes are machine names for URLs.
I theme often, so I'm a "themer".
is not how I think as a themer - it's how I think as a module developer, but that's a different role trying to achieve a different thing.
What I want when I theme is when I use url('thing/here') I am saying "I want the system to take users to the right place, without me needing to know in advance what "right place" means and that 'thing/here' should continue to resolve indefinitely and never break, no matter what modules or module updates or customisations may occur in the backend (unless the machine name itself is changed)" - the idea of prefixing with ROOT and language prefixes is not the themer's concern and is a "back end" implementation detail. The themer is never considering what permutations need to be considered by the system at all and should never have to - If we're claiming they do need to care "because Drupal" then we're making Drupal's DX worse than it has to be for no good reason.
Comment #128
thedavidmeister CreditAttribution: thedavidmeister commentedThere's a school of thought that very much wants
<a>
tags to appear inside templates as the "norm", for a list of good reasons including DX, simplicity, learnability, performance, and overall maintainability.The reason that now and in the immediate future most links will be fed by the menu system or through "fully rendered" #type => 'link' arrays is due to limitations that currently exist in Drupal, not because there is a majority consensus that has decided the ideal theme solution for links in Drupal should rely solely on precompiled data structures.
We should be trying to move Drupal away from anything that enforces those limits and towards supporting "raw" A tags in templates with dynamic attributes as soon as possible.
Comment #129
joelpittetWell maybe I'm wrong and/or conditioned to thinking I can put in a path in a url() and get back what I'd expect and routes are the way to go.
Assuming we go with url/path = routes absolute/relative like symfony's twig extension. Do we need to support what url() the way it exists at all in twig and can we make the API change to remove url() from core ala #2073811-84: Add a url generator twig extension @dawehner and @thedavidmeister?
Things change fast, it's hard to keep up and keep track...
Comment #130
dawehnerI totally think that url_from_path or something like this is a helpful functionality, especially if you deal with external URLs and you need to deal with query parameters etc.. In general I am a huge fan of explicitness and no magic.
Comment #131
dawehnerAdding some tests as this will be really bad for module developers
Comment #132
dawehnerReroll, I REALLY want this in. This is a requirement for devel.
Comment #134
pwolanin CreditAttribution: pwolanin commentedShould we handle Url objects too?
Comment #135
joelpittetThese extra filters and autoescape will be on top of my priorities at the con and extended sprints.
@pwolanin, if the url objects have decent public api with methods like getMyGetter() or just myGetter() and you pass a url object to the template you can just call {{ url.myGetter }} and {{ url.myGetter(arg, arg, etc) }} not sure if that is helpful or not?
Comment #136
joelpittetFor reference regarding my notes in #135
http://twig.sensiolabs.org/doc/recipes.html#using-dynamic-object-properties
And in the actual code:
https://github.com/fabpot/Twig/blob/4a7ef0ce0f4ca8d76a302a6d409f526a8309...
https://github.com/fabpot/Twig/blob/4a7ef0ce0f4ca8d76a302a6d409f526a8309...
Comment #137
pwolanin CreditAttribution: pwolanin commented@joelpittet - I think you can just call toString() on them, though in that case we are using the link generator, so I'm not sure how that's being addressed in Twig.
Comment #138
pwolanin CreditAttribution: pwolanin commentedWe need to revive this issue. This other Twig conversion just went in which is really poor since it hard-codes the A tag content: #1987424: seven.theme - Convert theme_ functions to Twig
Comment #139
joelpittet@pwolanin re
I feel no problem with that
<a>
tag in a template, it's much easier to manipulate markup if it's not coming from preprocess. Though I agree it's not right that we had to do that in the preprocess and couldn't in the template as well.{{ url_from_path(item.link_path) }}
or{{ url('node.add', { 'node_type': item.type }) }}
and{{ url('node.type_add') }}
As actual use-cases from #1987424: seven.theme - Convert theme_ functions to Twig
Though it sounds like url() PHP function is being deprecated from what you are telling me on IRC @pwolanin?
Also regarding the patch in #74 Are the UrlGenerator services really not available during install?
Comment #140
pwolanin CreditAttribution: pwolanin commentedWe should also consider a Twig function to render a Url object.
Comment #141
star-szrIf the Url object has a __toString() method then that is handled. And it looks like it does.
Comment #142
star-szrNevermind, it's toString() not __toString() (thanks @tim.plunkett in IRC!).
Comment #143
steveoliver CreditAttribution: steveoliver commentedRerolled #132
Comment #145
dawehnerWorking on fixing the failures.
Comment #146
Wim LeersI've read the entire issue.
I think #70, #78, #81, #83, #87 are the most important comments.
For quite some time now, we've been stuck on bikeshedding details. I think there is now general agreement on the minimum viable product:
url()
(for routes -> absolute URLs) andpath()
(for routes -> relative URLs). Everything else should be considered out of scope if we want this issue to be ever resolved:url()
+path()
) is what Twig+Symfony uses outside of Drupal, we should also support this, to ease the transition into Drupal. By stating we want this to work the same in Drupal as elsewhere, I think we can in good conscience let a good part of the bikeshed we've had here be put to rest :)file_create_url()
in D7 themes.)So… let's get this done and move on to more important matters!
Code review: mostly nitpicks.
s/URL path/URL path (relative URL)/
s/parameters/route parameters/
s/html/HTML/
Aren't these route parameters instead of query parameters? Hence aren't these examples all wrong?
s/But/but/
The respective descriptions should be on the next lines.
s/function/functions/
These are fundamentally wrong, but already were fundamentally wrong.
They must use a Twig function that calls Drupal's
file_create_url()
under the hood.Hence this can be done in a follow-up.
But, test coverage is missing for this as well.
Hmm… s/0/false/ ?
This reminds me of the bug I reported at https://github.com/symfony/symfony/issues/9604. It looks like this actually fixes the "bug" I reported. So this has been broken for at least 8 months :)
Comment #147
dawehnerThis piece of code talks about query parameters. For the path we have a static path pattern, so the URL can already be considered as safe.
Note: This is coming directly from symfony itself.
Yeah I realized that but this patch does not make it worse. Follow up: #2308187: Provide a twig extension for file_create_url
Yeah, isn't that nice?
Comment #148
Wim LeersMy remark in #146.4 was wrong; apparently this is possible:
I'll help with #2308187: Provide a twig extension for file_create_url.
With the basic use case solved by
url()
andpath()
, we could consider removingurl_from_path()
there (since its only use in this patch is for file URLs). But let's at least start moving forward again here :)RTBC.
Comment #149
star-szrI see this updates tablesort-indicator.html.twig, but there are other Twig templates using url(), wouldn't those need to be updated as well?
(edited to separate code blocks)
Comment #150
star-szrHappy to re-RTBC once #149 is addressed/answered…
Comment #151
pwolanin CreditAttribution: pwolanin commented<front>
is also a valid route name, so I think only 2 of those needs to be fixed?Comment #152
dawehner#2073811-149: Add a url generator twig extension
Good catch, this changed since the initial versions of the patch.
Comment #153
star-szrLooking at the interdiff I thought it was backwards at first but then I saw it's changing the seven theme and adding
url('admin/structures/types/add')
. I like that it makes it more consistent with the base template but doesn't that need to be changed to a route and the base node-add-list.html.twig updated to use a route as well?Comment #154
star-szrI deleted 'page' and 'article' content types and visited /node/add with #152 applied:
Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("Route "admin/structure/types/add" does not exist.") in "core/themes/seven/templates/node-add-list.html.twig" at line 25. in Twig_Template->displayWithErrorHandling() (line 291 of core/vendor/twig/twig/lib/Twig/Template.php).
Or using stark as the admin theme:
Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("Route "admin/structure/types/add" does not exist.") in "core/modules/node/templates/node-add-list.html.twig" at line 27. in Twig_Template->displayWithErrorHandling() (line 291 of core/vendor/twig/twig/lib/Twig/Template.php).
Comment #155
star-szrHow about this?
Comment #156
dawehnerYeah I am an idiot! Back to RTBC
Comment #157
alexpott@dawehner well you are an idiot who has exposed missing test coverage of node/add when we have no node types configured. Created #2309673: Add tests for node/add when no node types exist.
I don't think this is required anymore. The installer works just without the if.
This does not appear to be tested.
$link_generator is unused.
Comment #158
joelpittetThank you for carrying on without me, I'm wary of this change still but I think we can fix those issue once the come and it would be best to just get this into core. Thanks again.
A note:
This is actually wrong either way and a bug in core I think? I think that would prefix the variables with /fr-ca/ on a multilingual site. We should be using file_create_url()? Which I think we desperately need in the twig templates, but hopefully a better name. There is an issue for getting those in, but maybe we should add a test as well? @see #2168231: Twig Functions needed in templates @Cottser maybe you could help me out with building a test for multilingual to expose that bug?
Comment #159
joelpittetAsking for a revert on my blunder on the url() in tablesort indicator over at #2152261: Clean up for tablesort-indicator.html.twig
Comment #160
joelpittetAdded that revert I mentioned in #159 as well as #157.1 and #157.3. Leaving the tests still to be added. And depending on the revert getting committed first a re-roll.
Also removed the keys on the additional filters to be consistent with all the other Twig_SimpleFunctions
Comment #162
joelpittet@alexpott seems to be failing on #157.1 the installer bit. Any insight into this?
Reverted the reverted on that one for now.
Comment #163
dawehner@joelpittet
I think alex did not asked to just drop it completly :p Thanks for fixing the file_create_url() issue in a proper way.
@alexpott
Provided unit test coverage for the method.
Comment #164
star-szrWhat is this empty div in the test Twig file for? Can we remove it?
Updated the patch for some minor docs things, see interdiff.
Comment #165
dawehner+1 for the interdiff.
Ups, sure drop it.
Comment #166
joelpittetI obviously didn't understand what @alexpott meant by without it. Oh well.
Thanks for the doc fixes @Cottser and I've added your fixes to the new issue #2310311: Fix for tablesort-indicator.html.twig image urls for tablesort indicator urls.
Comment #167
joelpittetRemoved the div, very minor.
Back to RTBC as @dawehner and @Cottser fixed up the other items from #157
I'm for this patch:
URL building in templates is quite rare so issue with this approach will be a bit tricky to see until people use it, but generally its a big step forward and I'm confident we can solve any issues that arise in follow ups.
Comment #168
alexpottLooks like the added line is here is completely unnecessary - sorry I should have caught this before.
Why do we bother doing the set? For translation?
Comment #169
dawehnerIn general this makes the template more like the original in node module. Why it was done in the first place
I also can just bet: better translation support.
I cannot believe you until the bestbot says so.
Comment #170
dawehnerAs alex said, the port is set below already.
Comment #171
steveoliver CreditAttribution: steveoliver commentedLooks great!
Comment #172
alexpottCommitted 1a7cd1b and pushed to 8.x. Thanks!
Comment #175
star-szrRetroactively tagging.