Add a generateFromRoute() method on the url generator to accept options like url()

A different solution would have been to hack around the generate method to support a options array, but this will cause issues with understanding it.

I would rather see that happening.

Have been having a long conversation about this w/ dawehner and before him with alexpott in IRC.

For me, hacking the 3rd param to generate() would be ok. alexpott didn't like it.

dawehner points out that in Symfony\Cmf they are already hacking/overriding the meaning of the params to generate() in ContentAwareGenerator, so we would be in less violation of the interface than that code.

changing title

This adds an overriding interface, marks some things deprecated, and tweaks url() to take path as an array

@dawehner - yes, probably so unless we want to handle the query string independently to avoid the Symfony feature(?) that seems to prevent you from having a path slug and query parameter with the same name.

As @pwolanin pointed out in IRC (per @dawehner) the 'query' stuff is just insane. There's no way to generate the URL http:example.com/node/1?node=foo, for example.

On a more general note: If we're adding a separate interface anyway, what exactly is the benefit of hacking the existing function instead of providing a new one. I might be missing something but I literally see no benefit of that. Avoiding the 'query' madness would be only a nice side-effect of that.

Patch changes direction a little based on converstation with Crell & msonnabaum in IRC. Let's not blow up the exisintg method signature, but add a new method.

From Crell, we are trying to meet the basic requirements:

1. Given a route name, generate a domain-relative URL. Must include query parameters and fragments, and include outbound path manipulation (eg, for aliases and language prefixing).
2. Given a route name, generate an absolute URI. Must include query parameters and fragments, and include outbound path manipulation (eg, for aliases and language prefixing)
3. Given a URI (absolute or otherwise) generate an <a> element for it (containing various sytem-controlled attributes in the element).

The last would be handled by using this new code with a link generator for routes.

The doxygen change to l() will be reverted in the next patch - that was left-over from the rejected changed to url()

Adds tests and moves the interface name to UrlGeneratorWithOptionsInterface

Also, per discussion in IRC with dawhener and Crell, removes $parameters from being passed to getRouteByName() since the neither the Drupal implementation nor any symfony implementation look at them, they make no sesne being there in the interface (see getRoutesByName()) and they make the test mocking harder.

@dawehner - the generator doesn't handle external URLs, only absolute. the link generator would do that.

Other fixes made as suggested.

This looks pretty reasonable to me - can we not just call the interface UrlGeneratorInterface though? I mean, it's a different namespace, who cares that it supports options?

Also noticed some things about the test changes...

+++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.phpundefined
@@ -65,7 +72,7 @@ function setUp() {
     // getRoutesByNames() method calls on the route provider. The parameters
-    // passed in will be slightly different but based on the same information.
+    // are not passed in and default to an empty array.

This sentence was referring to the fact that the params for getRouteByName and getRoutesByNames are slightly different (i.e. one takes a single name and the other takes an array of names) - it was not referring to the "parameters" param. And if we don't need the "parameters" param any more, let's not include it in the $return_map_values array and just pass an empty array in the foreach loop.

As for the "Incorrect URL generated" messages, it's not clear to me that they're adding any value at all - let's just leave them out?

Fine for me to use the same interface name if it's not confusing?

updated per suggestions

It does need a simple DB query if the route has not been previously loaded - to the extent that we are using this in combination with access checks for rendering, I think it will be net faster.

re-roll for conflicts due to a change in ShortcutSetController.php

Tagging since this issue is changing an API and for the fact that this needs to be documented in the issue summary

The summary has been updated, and this change would make it a lot easier to finish #2047619: Add a link generator service for route-based links , so bumping to major

Suppose a bit more flexibility is cheap

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -23,7 +23,7 @@
+class UrlGenerator extends ProviderBasedGenerator implements UrlGeneratorInterface {

@@ -157,18 +157,41 @@ protected function getInternalPathFromRoute(SymfonyRoute $route, $parameters = a
+  public function generateFromRoute($name, $parameters = array(), $options = array()) {
...
+    $path = $this->processPath($path, $options);

suppose better to make this optional to allow apply only static route list

#26: routing-2057155-26.patch queued for re-testing.

Re-roll for conflicts in 2 form classes where use statements and constructors changed.

only "Stalking Crell" for a +1 - I don't think we need any technical input.

I'll admit I don't really understand the "bigger picture" that this is all leading to yet (despite pwolanin patiently trying to explain it to me; I think I might get it the third or fourth time :)), and I'm extremely concerned that it's going to push an initiative already wildly behind schedule even further behind — we need to be *very* critical about what we deem actual release-blocking issues at this point when we are adding far more of them per week than we are fixing, as is the current state of things.

However, this change itself is fairly straight-forward, so I don't think there's much harm in allowing it in; it simply makes this existing code consistent with the url() function. Tagging as an approved API change.

Here's my review:

+++ b/core/includes/common.inc
@@ -1192,7 +1191,7 @@ function datetime_default_format_type() {
- * @see \Drupal\Core\Routing\PathBasedGeneratorInterface::generateFromPath().
+ * @see \Drupal\Core\Routing\UrlGeneratorInterface::generateFromPath().

Most of the patch is this.

I asked Peter about the nature of this change; basically, because the inheritance hierarchy shifts to extending from Symfony CMF's "VersatileGeneratorInterface" (silliest. name. ever.) which itself descends from *their* version of UrlGeneratorInterface, it makes sense to rename this, because it's effectively our own version of theirs. (I also think it's confusing to have two classes named the same thing that do different things, but as has been pointed out in other issues, namespaces are the differentiator there.)

It shouldn't affect module developers too badly, although I see it does require changes on some entity listings.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -108,7 +108,7 @@ public function setRequest(Request $request) {
-    $route = $this->getRoute($name, $parameters);
+    $route = $this->getRoute($name);

I inquired about this too; $parameters isn't used and is deprecated upstream, so we're just bringing our code inline.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -157,18 +157,41 @@ protected function getInternalPathFromRoute(SymfonyRoute $route, $parameters = a
+  public function generateFromRoute($name, $parameters = array(), $options = array()) {

+++ b/core/lib/Drupal/Core/Routing/UrlGeneratorInterface.php
@@ -0,0 +1,201 @@
+   * @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)

$parameters and $options are very close synonyms, so initially I pushed back on this. But $parameters comes from Symfony CMF parlance (meaning either path slugs or query string arguments, as described below), and $options is the standard thing we use in Drupal to say "optional things". So I think this is ok.

+++ b/core/lib/Drupal/Core/Routing/UrlGeneratorInterface.php
@@ -0,0 +1,201 @@
+/**
+ * Defines an interface for generating a url from a route (or path) with
+ * additional options not present in the base generator.
+ */

(minor) This needs to be only one line long. Fixed in commit.

+++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
@@ -65,33 +72,29 @@ function setUp() {
-      $route_name_return_map[] = array($values['route_name'], $values['parameters'], $values['return']);
-      $routes_names_return_map[] = array(array($values['route_name']), $values['parameters'], $values['return']);
+      $route_name_return_map[] = array($values['route_name'], array(), $values['return']);
+      $routes_names_return_map[] = array(array($values['route_name']), array(), $values['return']);

This change seems kinda weird to me. It seems like if we're getting rid of $parameters, we shouldn't need it in tests anymore ether. I don't really understand this part of the code though, but maybe this needs a follow-up?

Apart from that, not much to complain about.

Committed and pushed to 8.x. Thanks!

change notice: https://drupal.org/node/2072005

/me inserts a +1 based on a read-through of #33.

Belatedly, it would seem... :-)