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:

  1. Retrieving a URL to a page from a "path" is deprecated in D8. We should not use them. Period.
  2. Provide one consistent way to create URLs for pages, using routes.
  3. Repurpose the {{ url() }} Twig function to be more in line with Symfony's, using routes. This outputs an absolute URL for the given route.
  4. Add a {{ path() }} Twig function, similar to Symfony's. This outputs a relative URL for the given route.
  5. This will ensure people learn the correct way (routes) to create URLs for pages and Symfony people will already know how to do it.
  6. 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.
  7. Create follow up issues (done, see below) to make creation of routes simpler by adding a route discovery mechanism.
  8. 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:

  1. A route, parameters and options.
  2. 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.

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.
CommentFileSizeAuthor
#170 interdiff.txt464 bytesdawehner
#170 twig_url-2073811-171.patch17.81 KBdawehner
#169 2073811-FAIL.patch18.26 KBdawehner
#167 add_a_url_generator-2073811-167.patch18.26 KBjoelpittet
#167 interdiff.txt954 bytesjoelpittet
#164 interdiff.txt1.71 KBstar-szr
#164 2073811-164.patch18.28 KBstar-szr
#163 interdiff.txt4.97 KBdawehner
#163 twig_url-2073811.patch18.1 KBdawehner
#160 add_a_url_generator-2073811-160.patch14.32 KBjoelpittet
#160 interdiff.txt5.53 KBjoelpittet
#155 interdiff.txt1.53 KBstar-szr
#155 2073811-155.patch14.95 KBstar-szr
#152 interdiff.txt1.18 KBdawehner
#152 twig-2073811-152.patch14.03 KBdawehner
#147 twig-2073811-147.patch12.85 KBdawehner
#147 interdiff.txt4.36 KBdawehner
#145 interdiff.txt3.14 KBdawehner
#145 twig-2073811-145.patch12.79 KBdawehner
#143 url_generator-2073811-143.patch11.19 KBsteveoliver
#132 url_generator-2073811-132.patch11.5 KBdawehner
#74 url_generator-2073811-74.patch11.76 KBpwolanin
#74 2073811-65-74.increment.txt8.42 KBpwolanin
#65 drupal-add-a-url-generator-2073811-65.patch9.83 KBmarkhalliwell
#65 interdiff.txt8.29 KBmarkhalliwell
#57 url_generator-2073811-57.patch10.36 KBdawehner
#57 interdiff.txt1.07 KBdawehner
#36 url_generator-2073811-36.patch11.42 KBpwolanin
#36 2073811-34-36.increment.txt3.67 KBpwolanin
#34 url_generator-2073811-34.patch11.42 KBdawehner
#34 interdiff.txt696 bytesdawehner
#29 twig-2073811-29.patch11.43 KBdawehner
#29 interdiff.txt1.77 KBdawehner
#26 twig_url-2073811-24.patch11.09 KBdawehner
#26 interdiff.txt5.67 KBdawehner
#12 twig-url_generator-2073811-12.patch11.04 KBpwolanin
#12 2073811-8-12.increment.txt5.92 KBpwolanin
#10 twig-2073811-10.patch9.69 KBdawehner
#8 twig-url_generator-2073811-8.patch9.16 KBpwolanin
#8 2073811-5-8.increment.txt2.07 KBpwolanin
#6 twig-url_generator-2073811-5.patch8.75 KBpwolanin
#6 2073811-4-5.increment.txt999 bytespwolanin
#4 twig-url_generator-2073811-4.patch8.63 KBpwolanin
#4 2073811-2-4.increment.txt6.92 KBpwolanin
#2 twig-url_generator-2073811-2.patch7.01 KBdawehner
#1 twig-url_generator-2073811-1.patch44.82 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs work
FileSize
44.82 KB

I need some help with as this does not work yet.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.01 KB

This was way too much.

Status: Needs review » Needs work

The last submitted patch, twig-url_generator-2073811-2.patch, failed testing.

pwolanin’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
6.92 KB
8.63 KB

Got 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.

Status: Needs review » Needs work

The last submitted patch, twig-url_generator-2073811-4.patch, failed testing.

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
999 bytes
8.75 KB

oops - can't access those services during install - so absolutely need to use sett injection in any case.

Status: Needs review » Needs work

The last submitted patch, twig-url_generator-2073811-5.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
2.07 KB
9.16 KB

silly me - the tests run in a subdir. Let's just use the proper generators, even though it's a bit of a silly test.

Status: Needs review » Needs work

The last submitted patch, twig-url_generator-2073811-8.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.69 KB

I need some debug informations from the testbot.

pwolanin’s picture

Status: Needs review » Needs work

The 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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.92 KB
11.04 KB

Here'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.

dawehner’s picture

Status: Needs review » Needs work

Opened a fix for that in upstream.

dawehner’s picture

Status: Needs work » Needs review
dawehner’s picture

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -197,10 +197,17 @@ public function generateFromRoute($name, $parameters = array(), $options = array
+      $http_port = $this->context->getHttpPort();
+      if ($http_port && $http_port != 80) {
...
+      $https_port = $this->context->getHttpsPort();
+      if ($https_port && $https_port != 443) {

We could just inline that variable assignment

pwolanin’s picture

@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

dawehner’s picture

I am wondering whether we should add explicit test coverage for the simpletest runner change.

pwolanin’s picture

@dawehner - it's a bit hard unless you're running the tests not on port 80?

Fabianx’s picture

Generally 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.

dawehner’s picture

Generally 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.

Well, 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?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

.

pwolanin’s picture

registerTwig() has another check on drupal_installation_attempted(), which is where I borrowed the logic, so somehow seems things are not complete during install

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

Dries’s picture

We 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?

pwolanin’s picture

@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

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.67 KB
11.09 KB

We 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?

Symonfy 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:

  • url() => url_from_path The url generator interface provides generateFromPath as method
  • get_url => url() Same structure as symfony
  • get_link => link()
pwolanin’s picture

There 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:

 'url_from_path' => new \Twig_Function_Function('url'),

should be like:

'url_from_path' => new \Twig_SimpleFunction('get_url', array($this, 'generateFromPath')),

and use the UrlGenerator object directly also.

Status: Needs review » Needs work

The last submitted patch, twig_url-2073811-24.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
11.43 KB

Good idea. This also fixes the test failures ...

Status: Needs review » Needs work
Issue tags: -MenuSystemRevamp, -WSCCI

The last submitted patch, twig-2073811-29.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#29: twig-2073811-29.patch queued for re-testing.

pwolanin’s picture

#29: twig-2073811-29.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +MenuSystemRevamp, +WSCCI

The last submitted patch, twig-2073811-29.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
696 bytes
11.42 KB

That's it.

Status: Needs review » Needs work

The last submitted patch, url_generator-2073811-34.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.67 KB
11.42 KB

The route name changed.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Good catch!

Fabianx’s picture

So 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.

dawehner’s picture

alexpott’s picture

#36: url_generator-2073811-36.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -197,10 +197,17 @@ public function generateFromRoute($name, $parameters = array(), $options = array
-    if ('http' === $scheme && 80 != $this->context->getHttpPort()) {
-      $port = ':' . $this->context->getHttpPort();
-    } elseif ('https' === $scheme && 443 != $this->context->getHttpsPort()) {
-      $port = ':' . $this->context->getHttpsPort();
+    if ('http' === $scheme) {
+      $http_port = $this->context->getHttpPort();
+      if ($http_port && $http_port != 80) {
+        $port = ':' . $http_port;
+      }
+    }
+    elseif ('https' === $scheme) {
+      $https_port = $this->context->getHttpsPort();
+      if ($https_port && $https_port != 443) {
+        $port = ':' . $https_port;
+      }

Is this actually necessary? Especially given that https://github.com/symfony/symfony/pull/8903 has been closed with no fix?

Doesn't this change

+++ b/core/scripts/run-tests.sh
@@ -301,6 +301,7 @@ function simpletest_script_init($server_software) {
+  $_SERVER['SERVER_PORT'] =  isset($parsed_url['port']) ? $parsed_url['port'] : 80;

Fix it?

Setting back to "needs review" to get an answer.

pwolanin’s picture

The change to run-tests.sh should be sufficient at least for this patch (I think - will test)

markhalliwell’s picture

Status: Needs review » Needs work
Issue tags: +Twig, +theme system cleanup
  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -17,11 +19,41 @@
    +      'url_from_path' => new \Twig_SimpleFunction('url_from_path', array($this, 'generateFromPath')),
    +      'url' => new \Twig_SimpleFunction('url', array($this, 'generateUrl')),
    

    I 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.

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -17,11 +19,41 @@
    +      'link' => new \Twig_SimpleFunction('link', array($this, 'generateLink'), array('is_safe' => array('html'))),
    

    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?

dawehner’s picture

I 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.

I 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.

Why is this function needed? If we're already at the template level, why can't we just create our own 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?

This is for sure not needed but it seems to be more consistent to have support for both.

markhalliwell’s picture

I am not sure whether an additional parameter makes it easier as a single method with an additional parameter.

I 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 the generateUrl 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.

This is for sure not needed but it seems to be more consistent to have support for both.

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 for url()).

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 convert link() to because I don't want that extra markup. So instead of introducing a whole new link() 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.

mortendk’s picture

+ 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 :)

steveoliver’s picture

Status: Needs work » Needs review

Aside from the unadressed items from @alexpott's review in #41,

I think I like url_from_path. Here's why.

{{ url_from_path('user/register') }}

{{ url('user/register', {'fromPath': 1}) }}
  1. Whereas the '_from_path' variant makes it immediately clear to me the special behavior of the first function, the "fromPath" arg (or any other such flag name and value we could come up with) I think could be more difficult to immediately understand.
  2. maybe silly, but _from_path = 10 extra characters; fromPath arg = 17 extra characters.
  3. Shouldn't we discourage building links from paths?
markhalliwell’s picture

Status: Needs review » Needs work

Yes, I agree that #41 needs work according to the GH PR.

Regarding url_from_path vs url(..., {'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.

steveoliver’s picture

Yeah I guess that does make sense if we don't want people generating urls from paths.

webchick’s picture

Hm. 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.

pwolanin’s picture

Indeed - 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.

markhalliwell’s picture

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.)

This 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.

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?

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.

we sure be discouraging the use of system paths

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.

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.

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).

dawehner’s picture

This is might what i want today
{{ node.titel}}

but in 3 months im gonna work on a project where i only need the
{{ node.nid }}{{node.title}}
cause "something something" happend - we never know what happens in the frontend, just that it will change :)

... pretty pretty please just the data :)

I 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.

markhalliwell’s picture

@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

thedavidmeister’s picture

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 for url()).

@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:

  • Some way of converting structured data (routes and paths) into string URLs that a web browser can use inside Twig
  • A way to use the URLs we generate to create HTML links that a web browser understands inside Twig

What we have outside Twig, in '#type' => 'link':

  • Automatic detection and handling of whether system data representing URLs is a route or a path - this is done through the name of parameters
  • Automatic handling of the hreflang attribute if we know the language of our URL
  • Automatic handling of the .active class based on the current URL
  • The ability to add extra query parameters *in addition to* the route/path
  • The ability for the structure of a link to be altered by modules before the link markup is rendered
  • Sanitisation of the components of a link (title attributes, etc..)
  • Integration with the AJAX framework

This 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') or url(parameters...), language and query handling hasn't come up. I'd like to hear from proponents of the former what the plan there is?

thedavidmeister’s picture

Oh, 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.

dawehner’s picture

I 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.

but we don't want the theme system to have to have "duplicate" functionality to handle paths and routes separately.

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.

Whereas the '_from_path' variant makes it immediately clear to me the special behavior of the first function, the "fromPath" arg (or any other such flag name and value we could come up with) I think could be more difficult to immediately understand.
maybe silly, but _from_path = 10 extra characters; fromPath arg = 17 extra characters.
Shouldn't we discourage building links from paths?

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).

On that note, I'm surprised that, in the discussion of whether to go with url('just a string') or url(parameters...), language and query handling hasn't come up. I'd like to hear from proponents of the former what the plan there is?

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.

   * @param string $name
   *   The name of the route
   * @param array  $parameters
   *   An associative array of parameter names and values.
   * @param array $options
   *   (optional) An associative array of additional options, with the following
   *   elements:
   *   - 'query': An array of query key/value-pairs (without any URL-encoding)
   *     to append to the URL. Merged with the parameters array.
   *   - 'fragment': A fragment identifier (named anchor) to append to the URL.
   *     Do not include the leading '#' character.
   *   - 'absolute': Defaults to FALSE. Whether to force the output to be an
   *     absolute link (beginning with http:). Useful for links that will be
   *     displayed outside the site, such as in an RSS feed.
   *   - 'language': An optional language object used to look up the alias
   *     for the URL. If $options['language'] is omitted, the language will be
   *     obtained from language(Language::TYPE_URL).
   *   - 'https': Whether this URL should point to a secure location. If not
   *     defined, the current scheme is used, so the user stays on HTTP or HTTPS
   *     respectively. if mixed mode sessions are permitted, TRUE enforces HTTPS
   *     and FALSE enforces HTTP.
   *
   * @return string
   *   The generated URL for the given route.
   *
   * @throws \Symfony\Component\Routing\Exception\RouteNotFoundException
   *   Thrown when the named route doesn't exist.
   * @throws \Symfony\Component\Routing\Exception\MissingMandatoryParametersException
   *   Thrown when some parameters are missing that are mandatory for the route.
   * @throws \Symfony\Component\Routing\Exception\InvalidParameterException
   *   Thrown when a parameter value for a placeholder is not correct because it
   *   does not match the requirement.
   */
  public function generateFromRoute($name, $parameters = array(), $options = array());

Given that, here is a patch which fixes the good catch by alex in #41.

thedavidmeister’s picture

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.

I'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.

Fabianx’s picture

Assigned: markhalliwell » Unassigned

So 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:

{{ path('node/1', is_path = true) }}
{{ url('node/1', is_path = true) }}

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:

with_query = {'key':val}, with_fragment = 'bottom'

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:

{{ link() }}

but also

{% set my_link = link('', 'node/1', is_path = true) %}
<a href="{{ link.url }}" {{ link.attributes }}>{{ link.content }}</a>

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.

markhalliwell’s picture

Title: Add a url generator and a link generator twig extension » Add a url generator twig extension
Assigned: Unassigned » markhalliwell

Agreed. Since no one seems willing, I'll create the Twig extensions.

dawehner’s picture

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.

I 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.

jibran’s picture

Assigned: Unassigned » markhalliwell
Status: Needs work » Needs review

Sending #57 to test.

markhalliwell’s picture

Hmm, 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:

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 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 function url() 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:

  1. Navigate to the url in question.
  2. Copy and paste that url (minus the base prefix)
  3. Use copied url for <?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 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.

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:

  public function generateUrl() {
    $args = func_get_args();
    // Try generating a route first.
    try {
      return call_user_func_array(array($this->urlGenerator, 'generateFromRoute'), $args);
    }
    // If not a route, try generating from a path instead.
    // This method is deprecated anyway. Only use it as a last resort.
    catch (Exception $e) {
      return call_user_func_array(array($this->urlGenerator, 'generateFromPath'), $args);
    }
  }
markhalliwell’s picture

markhalliwell’s picture

Providing patch

dawehner’s picture

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -97,30 +84,22 @@ public function getName()
+    try {
+      return call_user_func_array(array($this->urlGenerator, 'generateFromRoute'), $args);
+    }
+    // If provided string isn't a route, try generating from a path instead.
+    // This method is deprecated and is only used as a last resort.
+    catch (\Exception $e) {
+      return call_user_func_array(array($this->urlGenerator, 'generateFromPath'), $args);
+    }

You 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.

pwolanin’s picture

Status: Needs review » Needs work

Having 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:

{{ url(route_name = 'node.vew' ) }}

is the same as:

{{ url('node.vew' ) }}

But you can use the named href to get a path-based generator:

{{ url(href = 'user/login') }}
markhalliwell’s picture

Status: Needs work » Needs review
Issue tags: +FX (Front End Experience)

You 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.

Not really. It will still throw an exception at the path generation because "user.registar" isn't a valid path either.

Having to catch exceptions to handle a href seems really ugly.

What about passing in an array with named arguments, in analogy to #type => link?

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).

pwolanin’s picture

Status: Needs review » Needs work

No, 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.

markhalliwell’s picture

Assigned: markhalliwell » pwolanin

After 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.

pwolanin’s picture

Looking 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

thedavidmeister’s picture

After 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.

Last time I checked, the linkGenerator for routes could not handle absolute/external URLs. Is this still the case?

dawehner’s picture

Last time I checked, the linkGenerator for routes could not handle absolute/external URLs. Is this still the case?

I 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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
8.42 KB
11.76 KB

You 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.

dawehner’s picture

This 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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

.

webchick’s picture

Assigned: pwolanin » markhalliwell
Status: Reviewed & tested by the community » Needs review

I'd like to confirm this approach is ok with the front-enders.

+{# Test the url and path twig functions #}
+<div>path (as route) not absolute: {{ path('user.register') }}</div>
+<div>url (as route) absolute: {{ url('user.register') }}</div>
+
+<div>path (as route) not absolute with fragment: {{ path('user.register', {}, {'fragment': 'bottom' }) }}</div>
+<div>url (as route) absolute despite option: {{ url('user.register', {}, {'absolute': 0 }) }}</div>
+<div>url (as route) absolute with fragment: {{ url('user.register', {}, {'fragment': 'bottom' }) }}</div>

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?

webchick’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Yes, 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

Fabianx’s picture

I fully agree with #78. RTBC +1

steveoliver’s picture

Assigned: markhalliwell » Unassigned
Status: Reviewed & tested by the community » Needs review

Thanks, 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):

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -11,17 +11,45 @@
    +   * Constructs \Drupal\Core\Template\TwigExtension.
    

    This doesn't seem like quite the right title.

    Maybe "Sets generator services for the Drupal core Twig extension." ?

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -11,17 +11,45 @@
    +   * Returns a list of functions to add to the existing list.
    

    "Returns a list of functions made available to Drupal Twig templates."

  3. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -11,17 +11,45 @@
    +      // The url and path function are defined in close parallel to those found
    

    s/function/functions/.

  4. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -63,5 +91,83 @@ public function getName()
       }
    -}
     
    +
    +  /**
    +   * Generates a URL path given a route name and parameters.
    

    Looks like an extra new line here.

  5. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -63,5 +91,83 @@ public function getName()
    +   * @param $name
    

    typehint 'string' $name.

  6. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -63,5 +91,83 @@ public function getName()
    +   *   An associative array of parameter names and values.
    

    An associative array of route parameter names and values?

  7. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -63,5 +91,83 @@ public function getName()
    +  /**
    +   * Generates an absolute URL given a route name and parameters.
    +   * @param $name
    +   *   The name of the route.
    

    Needs an additional newline and 'string' typehint for $name.

  8. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -63,5 +91,83 @@ public function getName()
    +   *   An associative array of parameter names and values.
    

    An associative array of route parameter names and values?

  9. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -63,5 +91,83 @@ public function getName()
    +  public function isUrlGenerationSafe(\Twig_Node $argsNode)
    +  {
    

    Move { up on function line.

  10. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTestTwig.php
    @@ -47,4 +47,27 @@ function testTwigVariableDataTypes() {
    +    $link_generator = $this->container->get('link_generator');
    

    What's $link_generator doing in TestTwigUrlGenerator() ?

  11. +++ b/core/scripts/run-tests.sh
    @@ -301,6 +301,7 @@ function simpletest_script_init($server_software) {
    +  $_SERVER['SERVER_PORT'] =  isset($parsed_url['port']) ? $parsed_url['port'] : 80;
    

    This shouldn't be here. See comment See #14.

webchick’s picture

I'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. :(

<a href="foo/bar">blarg</a>

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:

<a href="{{ path('module.foo', {}, {'foo': 'bar'}) }}">blarg</a>

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?

steveoliver’s picture

After 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.

dead_arm’s picture

Much 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.

webchick’s picture

#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.

steveoliver’s picture

Then likewise a path() in PHP to return relative paths, since url() will always be returning absolute hrefs. +1.

steveoliver’s picture

The 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()

markhalliwell’s picture

Assigned: Dries » Unassigned
Status: Needs review » Reviewed & tested by the community

And 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):

  1. Make it as simple as possible, 2 functions (indentical in syntax) to Symfony's: url() (absolute) and path() (relative).
  2. These functions have the same parameter and option arguments. Let's not deviate from this (which, btw, is what's used internally by urlGenerate).
  3. Does core use "paths" internally? No. Don't provide a core Twig function for paths. "Paths" are deprecated, let's just NOT use them please.
  4. Don't give people more than one way to do something. We will hang ourselves with this extra rope and spend countless hours debating on which way is the "right" way (as we already have, instead of moving forward).
  5. If someone really, really, really wants to use deprecated code then they can extend Twig themselves to provide this functionality. I myself have already started a list of useful "functions" that only I would probably use (or allow my contrib modules to enhance templates, ie: Icon API).

-------

@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:

When I need to add a link that is within the site, it is pretty rare.

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.)

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.

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 :)

I actually like the idea of having a machine name that won't change

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):

url('user.register') => /user/register
url_from_path('user/register') => /user/register
url('user.register', {}, {'absolute: 1}) => http://example.org/user/register
url_from_path('user/register', {'absolute': 1}) => http://example.org/user/register
Themers that value their time and effort will appreciate being able to apply against non-brittle markup.

AMEN!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

markhalliwell’s picture

Assigned: Unassigned » Dries

Actually, I'm assigning to @Dries for review. We've all said our piece and played PONG. We need a final decision on this.

dawehner’s picture

Assigned: Unassigned » Dries
Status: Reviewed & tested by the community » Needs review

Therefore though I think someone should write a proper issue summary.

dawehner’s picture

thedavidmeister’s picture

I 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.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

Fabianx’s picture

There is another use case we did not think about:

* Non CSS-Images

D7:

print url('sites/all/themes/mytheme/myimg.png');

D8:

???

Maybe a compromise is to make base_path available within Twig?

That would allow the themer to write and learn:

<img src="{{ base_path }}/sites/all/themes/myimg.png" />

but it would also allow the novice themer to do:

<a href="{{ base_path }}/about-us" />

or

<a href="{{ base_path }}/user/register" />

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?

Fabianx’s picture

Issue summary: View changes

Update issue summary

webchick’s picture

{{ base_path }} is a great idea, and would very much help assuage my concerns! Thanks, Fabianx.

dawehner’s picture

{{ base_path }} is a great idea, and would very much help assuage my concerns! Thanks, Fabianx.

Right, sadly this bypasses url aliases, language prefixes/query arguments and what else.

xjm’s picture

Thanks @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!

Wim Leers’s picture

#92: no, using url() in D7 is completely wrong. You should use file_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.

xjm’s picture

Assigned: Dries » markhalliwell

Er like that. :)

Fabianx’s picture

#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.

Wim Leers’s picture

#98: agreed — wise words :)

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -11,17 +11,45 @@
-      // @todo Remove URL function once http://drupal.org/node/1778610 is resolved.
-      'url' => new \Twig_Function_Function('url'),

I'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,

Themers create links very seldomly directly in templates.

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?

The theme system would like to have _one_ consistent way of creating urls for links

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?

effulgentsia’s picture

Issue summary: View changes

Update comment to #92

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Assigned: markhalliwell » Dries

I'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.

effulgentsia’s picture

Thanks 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.

effulgentsia’s picture

Also, 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.

steveoliver’s picture

Assigned: Dries » Unassigned
Priority: Major » Normal
Status: Needs review » Postponed (maintainer needs more info)

Thank 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++

markhalliwell’s picture

Assigned: Unassigned » Dries
Priority: Normal » Major
Status: Postponed (maintainer needs more info) » Needs review

@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:

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?

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):

function mytheme_preprocess_node(&$variables) {
  // For just the URL, because we want a custom <a/> tag.
  $urlGenerator = new UrlGenerator();
  $variables['register_url'] =$urlGenerator->generateFromRoute('user.register', array(), array('absolute' => TRUE));
  // We don't care about what's in the <a/> tag.
  $linkGenerator = new LinkGenerator();
  $variables['register'] = $linkGenerator->generateFromRoute('user.register', array(), array('absolute' => TRUE));
}

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).

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 module://, theme:// and profile:// stream wrappers to access system files done, we'll at least be able to unify the syntax for uploaded files and shipped files)

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).

markhalliwell’s picture

Assigned: Dries » Unassigned
Priority: Major » Normal
Status: Needs review » Postponed (maintainer needs more info)

Ugh, fine. Let's make this a lot more complicated than it really has to be. I'm done with this issue. Un-followed.

dawehner’s picture

Thanks 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:

Well, 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.

pwolanin’s picture

We 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.

steveoliver’s picture

Thanks, 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

steveoliver’s picture

D'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

steveoliver’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

steveoliver’s picture

Status: Postponed (maintainer needs more info) » Needs review

OK, 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:

  1. Drupal uses the terms 'url', 'path', 'alias' in ways that are not as strict as Symfony (or don't exist, in the case of 'alias'). We have cruft around these ideas for which next steps are slowly becoming clarified. I don't see what we win by trying to enforce the distinction of url(absolute)/path(relative) in Drupal at this point, either in PHP or Twig. Symfony introduces the hard distinction between url() and path() in Twig. I'm just not convinced Drupal needs to.
  2. url() is what we've had and continue to use for both absolute and relative paths in all of Drupal. We should keep (and/or evolve) this behaviour in Drupal PHP and use it to back the same function {{ url() }} in Drupal Twig.
  3. url() may need to evolve or split to support entity operations and files (since they're based on paths still). Relate any such changes back to Twig, but in the meantime...
  4. If Twig templates need to reference a file's url, they should use the url/path directly (at least for now, unless url() or whatever else ultimately handles files).
  5. {{ base_url }} is a nice var that I think should be made available to Twig, that could be used in conjunction with manual paths to generate absolute urls to files, for example.

Setting to Needs review for others to chime in.

dawehner’s picture

I 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

thedavidmeister’s picture

#111 seems to make sense to me.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

So anyone here who would like to get this patch in, or should we just let people coming from symfony be confused?

steveoliver’s picture

My 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:

"I don't see what we win by trying to enforce the distinction of url(absolute)/path(relative) in Drupal at this point, either in PHP or Twig"

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.

dawehner’s picture

Should 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.

joelpittet’s picture

re: #92 Created an issue to get those paths in twig: #2168231: Twig Functions needed in templates

joelpittet’s picture

From 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:

<a href="{{ url_from_path('my/view-path') }}">...</a> => /my/view-path

And if I needed query params or some variation it would be like:

<a href="{{ url_from_path('my/view-path', { query: { sort: 'desc' }, absolute:  true }) }}">...</a> => http://example.org/my/view-path?sort=desc

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.

<a href="{{ url('user.register') }}">...</a> => /user/register

I'd prefer if the second argument took in all the options needed.

<a href="{{ url('user.register', { query: { sort: 'desc' }, absolute:  true }) }}">...</a> => http://example.org/user/register?sort=desc

FWIW

dawehner’s picture

As 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 :(

joelpittet’s picture

So to bump this issue: url_from_path() for paths, url() for routes is what I'm getting at in #118.

jenlampton’s picture

I 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.

joelpittet’s picture

As 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.

joelpittet’s picture

I 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!

dawehner’s picture

s. 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.

/me sighs

joelpittet’s picture

@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?

dawehner’s picture

Well, 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.

thedavidmeister’s picture

You want to be able to put routes in templates.

Routes are machine names for URLs.

I theme often, so I'm a "themer".

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".

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.

thedavidmeister’s picture

Most links will be fed by the menu system, in the body field or #type=>link.

There'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.

joelpittet’s picture

Well 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...

dawehner’s picture

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?

I 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.

dawehner’s picture

Adding some tests as this will be really bad for module developers

dawehner’s picture

Reroll, I REALLY want this in. This is a requirement for devel.

Status: Needs review » Needs work

The last submitted patch, 132: url_generator-2073811-132.patch, failed testing.

pwolanin’s picture

Should we handle Url objects too?

joelpittet’s picture

Issue tags: +sprint

These 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?

pwolanin’s picture

@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.

pwolanin’s picture

We 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

joelpittet’s picture

@pwolanin re

This other Twig conversion just went in which is really poor since it hard-codes the A tag content

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?

pwolanin’s picture

We should also consider a Twig function to render a Url object.

star-szr’s picture

If the Url object has a __toString() method then that is handled. And it looks like it does.

star-szr’s picture

Nevermind, it's toString() not __toString() (thanks @tim.plunkett in IRC!).

steveoliver’s picture

Status: Needs work » Needs review
FileSize
11.19 KB

Rerolled #132

Status: Needs review » Needs work

The last submitted patch, 143: url_generator-2073811-143.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.79 KB
3.14 KB

Working on fixing the failures.

Wim Leers’s picture

Status: Needs review » Needs work

I'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) and path() (for routes -> relative URLs). Everything else should be considered out of scope if we want this issue to be ever resolved:

  1. Let's not forget that given the descriptions in #83 and #87, I think it's safe to conclude that generating links in the theme layer (in Twig templates) is a very rare task. So let us please not waste any more time bikeshedding this issue.
  2. Given that this (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 :)
  3. Dealing with file URLs are out of scope. (And I say this as the maintainer of the CDN module, where I've seen lots of people doing lots of crazy wrong things because they weren't using file_create_url() in D7 themes.)

So… let's get this done and move on to more important matters!


Code review: mostly nitpicks.

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -80,4 +103,99 @@ public function getName() {
    +   * Generates a URL path given a route name and parameters.
    ...
    +   *   The generated URL path for the given route.
    

    s/URL path/URL path (relative URL)/

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -80,4 +103,99 @@ public function getName() {
    +   *   An associative array of parameter names and values.
    ...
    +   *   An associative array of parameter names and values.
    

    s/parameters/route parameters/

  3. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -80,4 +103,99 @@ public function getName() {
    +   * Thus, the only character within an URL that must be escaped in html is the
    

    s/html/HTML/

  4. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -80,4 +103,99 @@ public function getName() {
    +   * But the following may need to be escaped:
    +   * - path('route', var)
    +   * - path('route', {'param': ['val1', 'val2'] }) // a sub-array
    +   * - path('route', {'param1': 'value1', 'param2': 'value2'})
    

    Aren't these route parameters instead of query parameters? Hence aren't these examples all wrong?

  5. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -80,4 +103,99 @@ public function getName() {
    +   * need to be escaped, But we don't know that in advance.
    

    s/But/but/

  6. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -80,4 +103,99 @@ public function getName() {
    +   * @param \Twig_Node $argsNode The arguments of the path/url function
    +   *
    +   * @return array An array with the contexts the URL is safe
    

    The respective descriptions should be on the next lines.

  7. +++ b/core/modules/system/src/Tests/Theme/EngineTwigTest.php
    @@ -41,4 +41,27 @@ function testTwigVariableDataTypes() {
    +   * Tests the url and url_generate Twig function.
    

    s/function/functions/

  8. +++ b/core/modules/system/templates/tablesort-indicator.html.twig
    @@ -10,7 +10,7 @@
    -  <img src="{{ url('core/misc/arrow-asc.png') }}" width="13" height="13" alt="{{ 'sort ascending'|t }}" title="{{ 'sort ascending'|t }}" />
    +  <img src="{{ url_from_path('core/misc/arrow-asc.png') }}" width="13" height="13" alt="{{ 'sort ascending'|t }}" title="{{ 'sort ascending'|t }}" />
    ...
    -  <img src="{{ url('core/misc/arrow-desc.png') }}" width="13" height="13" alt="{{ 'sort descending'|t }}" title="{{ 'sort descending'|t }}" />
    +  <img src="{{ url_from_path('core/misc/arrow-desc.png') }}" width="13" height="13" alt="{{ 'sort descending'|t }}" title="{{ 'sort descending'|t }}" />
    

    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.

  9. +++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.url_generator.html.twig
    @@ -0,0 +1,7 @@
    +<div>url (as route) absolute despite option: {{ url('user.register', {}, {'absolute': 0 }) }}</div>
    

    Hmm… s/0/false/ ?

  10. +++ b/core/scripts/run-tests.sh
    @@ -336,6 +336,7 @@ function simpletest_script_init() {
    +  $_SERVER['SERVER_PORT'] = isset($parsed_url['port']) ? $parsed_url['port'] : 80;
    

    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 :)

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.36 KB
12.85 KB

Aren't these route parameters instead of query parameters? Hence aren't these examples all wrong?

This 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.

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.

Yeah I realized that but this patch does not make it worse. Follow up: #2308187: Provide a twig extension for file_create_url

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 :)

Yeah, isn't that nice?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

My remark in #146.4 was wrong; apparently this is possible:

$urlgenerator->generate('node.view', array('node' => 1, 'foo' => 'bar'))
generates
node/1?foo=bar


I'll help with #2308187: Provide a twig extension for file_create_url.


With the basic use case solved by url() and path(), we could consider removing url_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.

star-szr’s picture

I see this updates tablesort-indicator.html.twig, but there are other Twig templates using url(), wouldn't those need to be updated as well?

core/modules/node/templates/node-add-list.html.twig
27:    {% set create_content = url('admin/structure/types/add') %}
core/modules/system/templates/block--system-branding-block.html.twig
20:    <a href="{{ url('<front>') }}" title="{{ 'Home'|t }}" rel="home" class="site-logo">
26:      <a href="{{ url('<front>') }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
core/modules/system/templates/tablesort-indicator.html.twig
13:  <img src="{{ url('core/misc/arrow-asc.png') }}" width="13" height="13" alt="{{ 'sort ascending'|t }}" title="{{ 'sort ascending'|t }}" />
15:  <img src="{{ url('core/misc/arrow-desc.png') }}" width="13" height="13" alt="{{ 'sort descending'|t }}" title="{{ 'sort descending'|t }}" />
core/themes/bartik/templates/block--system-branding-block.html.twig
18:    <a href="{{ url('<front>') }}" title="{{ 'Home'|t }}" rel="home" class="site-logo">
26:          <a href="{{ url('<front>') }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>

(edited to separate code blocks)

star-szr’s picture

Status: Reviewed & tested by the community » Needs review

Happy to re-RTBC once #149 is addressed/answered…

pwolanin’s picture

<front> is also a valid route name, so I think only 2 of those needs to be fixed?

dawehner’s picture

FileSize
14.03 KB
1.18 KB

#2073811-149: Add a url generator twig extension
Good catch, this changed since the initial versions of the patch.

star-szr’s picture

Looking 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?

star-szr’s picture

Status: Needs review » Needs work

I 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).

star-szr’s picture

Status: Needs work » Needs review
FileSize
14.95 KB
1.53 KB

How about this?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah I am an idiot! Back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@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.

  1. +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
    @@ -81,6 +81,11 @@ public static function registerTwig(ContainerBuilder $container) {
    +    // When in the installer these services are not yet available.
    +    if (!drupal_installation_attempted()) {
    +      $twig_extension->addMethodCall('setGenerators', array(new Reference('url_generator')));
    +    }
    

    I don't think this is required anymore. The installer works just without the if.

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -88,4 +111,101 @@ public function getName() {
    +   * But the following may need to be escaped:
    +   * - path('route', var)
    +   * - path('route', {'param': ['val1', 'val2'] }) // a sub-array
    +   * - path('route', {'param1': 'value1', 'param2': 'value2'})
    

    This does not appear to be tested.

  3. +++ b/core/modules/system/src/Tests/Theme/EngineTwigTest.php
    @@ -41,4 +41,27 @@ function testTwigVariableDataTypes() {
    +    $link_generator = $this->container->get('link_generator');
    

    $link_generator is unused.

joelpittet’s picture

Thank 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:

+++ b/core/modules/system/templates/tablesort-indicator.html.twig
@@ -10,7 +10,7 @@
-  <img src="{{ url('core/misc/arrow-asc.png') }}" width="13" height="13" alt="{{ 'sort ascending'|t }}" title="{{ 'sort ascending'|t }}" />
+  <img src="{{ url_from_path('core/misc/arrow-asc.png') }}" width="13" height="13" alt="{{ 'sort ascending'|t }}" title="{{ 'sort ascending'|t }}" />
...
-  <img src="{{ url('core/misc/arrow-desc.png') }}" width="13" height="13" alt="{{ 'sort descending'|t }}" title="{{ 'sort descending'|t }}" />
+  <img src="{{ url_from_path('core/misc/arrow-desc.png') }}" width="13" height="13" alt="{{ 'sort descending'|t }}" title="{{ 'sort descending'|t }}" />

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?

joelpittet’s picture

Asking for a revert on my blunder on the url() in tablesort indicator over at #2152261: Clean up for tablesort-indicator.html.twig

joelpittet’s picture

Status: Needs work » Needs review
FileSize
5.53 KB
14.32 KB

Added 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

Status: Needs review » Needs work

The last submitted patch, 160: add_a_url_generator-2073811-160.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
15.72 KB

@alexpott seems to be failing on #157.1 the installer bit. Any insight into this?

Reverted the reverted on that one for now.

dawehner’s picture

FileSize
18.1 KB
4.97 KB

@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.

star-szr’s picture

FileSize
18.28 KB
1.71 KB
+++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.url_generator.html.twig
@@ -0,0 +1,9 @@
+
+<div></div>

What is this empty div in the test Twig file for? Can we remove it?

Updated the patch for some minor docs things, see interdiff.

dawehner’s picture

+1 for the interdiff.

What is this empty div in the test Twig file for? Can we remove it?

Ups, sure drop it.

joelpittet’s picture

I 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.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
954 bytes
18.26 KB

Removed the div, very minor.

Back to RTBC as @dawehner and @Cottser fixed up the other items from #157

I'm for this patch:

  • It pushes routes for consistency when adding URLs
  • It adds a way for people to still use the older paths generation(for as long as that stays in core)
  • It keeps things in line with Symfony's Twig bridge.
  • It seems to have a good amount of test coverage.
  • The names have been discussed(*cough* bikesheded) just the right amount;)

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/scripts/run-tests.sh
@@ -336,6 +336,7 @@ function simpletest_script_init() {
+  $_SERVER['SERVER_PORT'] = isset($parsed_url['port']) ? $parsed_url['port'] : 80;
...
   $_SERVER['SERVER_PORT'] = $port;

Looks like the added line is here is completely unnecessary - sorry I should have caught this before.

+++ b/core/themes/seven/templates/node-add-list.html.twig
@@ -22,8 +21,9 @@
+    {% set create_content = path('node.type_add') %}
...
+      You have not created any content types yet. Go to the <a href="{{ create_content }}">content type creation page</a> to add a new content type.

Why do we bother doing the set? For translation?

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.26 KB

Why do we bother doing the set? For translation?

In 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.

Looks like the added line is here is completely unnecessary - sorry I should have caught this before.

I cannot believe you until the bestbot says so.

dawehner’s picture

FileSize
17.81 KB
464 bytes

As alex said, the port is set below already.

steveoliver’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1a7cd1b and pushed to 8.x. Thanks!

  • alexpott committed 1a7cd1b on 8.x
    Issue #2073811 by dawehner, pwolanin, joelpittet, Cottser, Mark Carver,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

star-szr’s picture

Retroactively tagging.