Add a link generator service for route-based links

As talked on IRC, lets introduce a LinkGenerator.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.phpundefined
@@ -0,0 +1,133 @@
+    $this->moduleHandler->alter('route_link', $variables);

Needs api.php entry

• Added the request via a setter method, so the DIC will automatically renew the request object
• The active information about the page is stored in a variable now.
• Added some todos regarding using route names instead of paths to compare.

Tagging. Peter, please update.

.

Looks pretty cool overall. Some minor remarks:

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -0,0 +1,186 @@
+  protected $request;
+  /**

Missing newline.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -0,0 +1,186 @@
+    // Because l() is called very often we statically cache values that require an

"Because this service is called very often, we statically..."
(l() -> this service and missing comma)

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -0,0 +1,186 @@
+    // @todo - if we want to render in the non-current it seems like we'd have

Do you mean "in a language different from the current one" (i.e. the word language is missing?)?

Thank you for your review! I removed that todo, because it is using the path processor both in url_generator->generate() as well as url_generator->generateFromPath()

Additional here is a unit test.

Ups.

:(

Might be better as a follow-up, but it would be cool if we could add a Drupal::link($text, $route, ...) for use in procedural code.

Looks great now, but I don't feel comfortable RTBC-ing myself.

Here is the follow up: #2051813: Reuse the link generator to provide a Drupal::link() function

+++ b/core/includes/common.inc
@@ -1297,6 +1297,9 @@ function drupal_http_header_attributes(array $attributes = array()) {
+ * @deprecated as of Drupal 8.0 for internal links. Use route names with rl()
+ * instead, and only use l() for formatting and sanitizing external URLs.

There is no rl() function. This message seems out of date.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -0,0 +1,187 @@
+  public function render($text, $route_name, array $parameters = array(), array $options = array()) {

This may have been discussed above, but render() seems like a horribly obscure name for "gimme a link".

No idea about the naming issue, but my gut feeling was consistency with the url generator and use generate().

In general we have to figure out how we integrate external links. I guess we want to use a "renderExternalLink" or something like that method?

+++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.phpundefined
@@ -0,0 +1,152 @@
+namespace {
+  if (!function_exists('drupal_is_front_page')) {
+    function drupal_is_front_page() {
+      return FALSE;
+    }

@todo will help.

Added a todo. I guess the failures was just some randomness on the testbot.

It seems there is a clear consensus to move it to JS for l() and/or limit it to tabs and other navigation?

Can you link some issues for that? I haven't seen it yet. This would be great in terms of cachability, but things like the active menu tree will not be trivial. I would suggest to keep this out of the patch, as long we don't have a replacement for it.

We could also go with $link_generator->link() or $link_generator->generate(), l feels wrong, as it does not tell anything at all, unless you are familiar with Drupal before.

#1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links

Let's go with ->link for now.

+++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.phpundefined
@@ -79,7 +79,7 @@ public function setUpLanguageManager() {
+   * @see \Drupal\Core\Utility\LinkGenerator::render(link

@@ -101,17 +101,17 @@ public function testRender() {
+   * @see \Drupal\Core\Utility\LinkGenerator::render(link

Copy paste error.

I blame storm!

As far as I can see, that issue summary still has a big hole in it, so re-tagging. I'm especially curious why this is critical, and an API change being proposed well post-API freeze. It also seems like rl() is no longer the suggested fix, so the title likely needs an update as well.

I don't see any lines in this patch that actually remove a call to l() and replace it with ... whatever you would replace it with. So what would be the equivalent of this now:

$output .= l($node->label(), 'node/' . $node->id());

updated the summary

update the issue summary and now the title.

Technical seen this is kind of a api addition at that point but yeah on the longrun it should replace the system.

+++ b/core/includes/common.inc
@@ -1297,6 +1297,10 @@ function drupal_http_header_attributes(array $attributes = array()) {
+ * @deprecated as of Drupal 8.0 for internal links. Use route names with the
+ *   link generator service(link_generator) with its render method and only use
+ *   l() for formatting and sanitizing external URLs.

I think we can improve the wording here. Also the usage of @deprecated does not follow current standards. Suggestion:

@deprecated Deprecated as of Drupal 8.0 for internal links. Use \Drupal\Core\Utility\LinkGenerator::link() instead, which is exposed as the 'link_generator' service. It requires an internal route name. Only use l() for outputting links to external URLs.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -0,0 +1,187 @@
+

Superfluous newline.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -0,0 +1,187 @@
+   * @param array $parameters
+   *   Any parameters needed to render the route path pattern.

Is missing the "(optional) " part. I was about to remark that this could be a bit more descriptive about what the $parameters actually should be, but that is not documented at all in \Symfony\Component\Routing\Generator\UrlGeneratorInterface::generate(), so... :-(

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -0,0 +1,187 @@
+   *   An associative array of additional options. Defaults to an empty array. It
+   *   may contain the following elements.
+   *   - 'attributes': An associative array of HTML attributes to apply to the
...
+   *   - 'html' (default FALSE): Whether $text is HTML or just plain-text. For

Reading the code, I think 'query', 'absolute' and 'language' are supported as well.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -0,0 +1,187 @@
+    // Start building a structured representation of our link to be altered later.
...
+    $url = String::checkPlain($this->urlGenerator->generate($variables['route_name'], $variables['parameters'], $variables['options']['absolute']));
...
+    $this->moduleHandler->alter('route_link', $variables);

This needs API documentation. I was going to copy it from hook_link_alter(), but that is not documented anywhere! It's also very strange that the altering takes place so late, as e.g. changing the langauge of a link does not work anymore at this stage, but that's also taken from l() directly, so should be a separate issue. For instance calling $variables['route_name'] instead of $route_name directly makes no difference in the given snippet because the alter hook hasn't run yet.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -0,0 +1,187 @@
+      'text' => is_array($text) ? drupal_render($text) : $text,

Even though drupal_render() being a class is still miles away, since this is the only call to a procedural function in this class (unless I missed something) I think a @todo couldn't hurt.

+++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
@@ -0,0 +1,153 @@
+    $this->languageManager = $this->getMockBuilder('Drupal\Core\Language\LanguageManager')
+      ->disableOriginalConstructor()
+      ->getMock();

Why is the disableOriginalConstructor() call needed? For reference the constructor of LanguageManager is this:

  public function __construct(KeyValueStoreInterface $state = NULL) {
    $this->state = $state;
  }

+++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
@@ -0,0 +1,153 @@
+  public function testLink() {

We should also test the 'html' and the 'absolute' option.

+++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
@@ -0,0 +1,153 @@
+      ->will($this->returnValueMap(array(
+        array('test_route_1', array(), FALSE, '/test-route-1'),
+        array(
+          'test_route_2',
+          array('value' => 'example'),
+          FALSE,
+          '/test-route-2/example'
+        ),
+      )));

I think the indentation is off here. Also, I think both arrays should be formatted equally for legibility.

Also, what is this 'value' thing? If I get it correctly, this is testing that the $parameters get passed correctly to the URL generator. If so, there should be a comment for that.

+++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
@@ -0,0 +1,153 @@
+  public function testLinkActive() {

We cannot test that the active class is added on the front page for links to the front page because drupal_is_front_page() is not yet a service. There should be a @todo for that.

We should test however, that setting a different language removes the 'active' class and that different query parameters also remove the 'active' class.

+++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
@@ -0,0 +1,153 @@
+  // @todo Remove this once there is a service for drupal_is_front_page

Super minor, but missing parentheses after the function name.

I'm going to work on this a bit and also improve the title and issue summary.

Per @dawehner, we should also test that the hook_link_alter() is fired.

I also cross-posted with him, he already updated the issue summary and title.

Will work on the patch now.

Updated docs, did not add additional tests yet.

Also opened #2055711: Invoke hook_link_alter() earlier in l().

Yeah, that needs to be Drupal::link(), not Drupal::service('link_generator')->link(). That function gets called literally all the time, so definitely fits into the definition of public-facing API, and thus should have a first-level method.

readd the title.

Thanks for your review and fix!

Added a Drupal::linkGenerator() to match Drupal::urlGenerator().

This will need profiling as links are heavily used within sites.

So, how do I use this with the Render API?

This patch makes a l() "equivalent" without making equivalents for any of the "higher level" functionality that uses l() currently.

We're "halfway through" cleaning up our #type 'link' ish elements in the theme system:

#2043649: Make all #type 'link' arrays 100% renderable, use #theme_wrappers if necessary
#1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays
#2052253: [META] Add #render property to drupal_render() and convert #type "#pre_render -> #markup" calls to use it
#2044105: Introduce #alters for \Drupal\Core\Render\Renderer::render()

If l() is deprecated, what is the proposed "correct way" to render links through the Render API as #type 'link' will be using deprecated functionality with the current patch and is therefore presumably no longer "correct"?

Re-adding tag.

What about don't mark it as @deprecated for now, but wait until you have reworked theme_link and go, because
this allows us to make some progress in the meantime.

Yeah, I agree with the notion. I removed the @deprecated and added some inline comment that the function is discouraged for internal use.

I added a bunch of test cases. This gets us to 97% test coverage. The remaining one line that supposedly isn't covered is I think due to different possibilities of passing the $options parameter. I.e. you can pass 'language' and 'html', or 'absolute' and 'query', or all of them, ... I didn't go so far to test all possible combinations, I think that would be a bit academic.

Will profile this now.

EDIT: I also took the liberty of splitting up some long lines, I hope that doesn't put anyone off.

I'm also not sure how I feel about the alter using "route_link" instead of just "link"...

I understand that $route_name is different to $path but everything else should be identical to l().

The alter in l() was intended as a compromise for preserving flexibility of markup when we removed theme_link(), meaning that anyone who would be implementing that alter would be expecting it to apply to all links regardless of the API used to generate the displayed href.

Also, I have to ask, as I can't see anything in the summary or comments but do we really need a whole new function to meet the requirement of "remove reliance on Drupal system paths and instead work with routes and their parameters" - couldn't l() be reworked a little to try one method of generating the href from a path or route by setting a flag in $options to TRUE/FALSE?

maybe even if $options has 'parameters' set, just assume that it's a route?

The alter in l() was intended as a compromise for preserving flexibility of markup when we removed theme_link(), meaning that anyone who would be implementing that alter would be expecting it to apply to all links regardless of the API used to generate the displayed href.

We introduce a new alter hook, so that is not really a problem. Hopefully we will be able to remove l() on the longrun.

Also, I have to ask, as I can't see anything in the summary or comments but do we really need a whole new function to meet the requirement of "remove reliance on Drupal system paths and instead work with routes and their parameters" - couldn't l() be reworked a little to try one method of generating the href from a path or route by setting a flag in $options to TRUE/FALSE?

So, we want to clearly separate the current system from the new system, because having something like l('String', 'route_name', array('parameters' => array())) is really a problem. As said before, we slowly have to make progress and can't wait years to get it done right at the first place.

maybe even if $options has 'parameters' set, just assume that it's a route?

Please now.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.phpundefined
@@ -0,0 +1,194 @@
+class LinkGenerator {

missing documentation, damn

+++ b/core/modules/system/system.api.phpundefined
@@ -3489,6 +3489,46 @@ function hook_filetransfer_info_alter(&$filetransfer_info) {
+ *   - text: he link text for the anchor tag as a translated string or render

he => The

We introduce a new alter hook, so that is not really a problem.

No, that *is* the problem, it's not removing the problem >.< In that, if I want to modify the attributes (for example) for all links on the site I now have to implement two alters where currently I have to only implement one.

There's no documentation around the two different alters at all. Not even an inline comment in l() above its alter(). I believe it makes using the alters hard to use and harder to learn about in the first place.

On a related note, actually implementing the alter in core to achieve something that could be inlined into the new function does look like a performance hit for no extra benefit.

What about don't mark it as @deprecated for now, but wait until you have reworked theme_link and go, because
this allows us to make some progress in the meantime.

There's no theme_link anymore, there's only #type 'link' that uses l() in #pre_render to generate links. We're not "reworking", we're mid-late conversion based on something we already have reworked, which was removing everything that previously generated links in templates and theme functions (implementations which would not have cared so much about plans to remove l()) and converting them to all use l() for everything (which is now awkward).

For all practical purposes #type 'link' is just l() by another name. Why would we discourage l()'s usage here while simultaneously actively working to increase it's usage somewhere else?

+ * As of Drupal 8.0 this usage of this function for internal links is
+ * discouraged. Using \Drupal\Core\Utility\LinkGenerator::link(), which is
+ * exposed as the 'link_generator' service, is preferred. It requires an
+ * internal route name. l() should still be used for outputting links to
+ * external URLs.
+ *

I have no idea how #type 'link' should work any more. It is based on l() so therefore it is "discouraged" for internal links, but is required for external links... how could we know when defining an element type whether #type 'link' is going to be used for internal or external links?

The docs say nothing about what to do if I don't know whether the link I'm generating will have an internal or external href. Should #type 'link' be trying to detect "externality" based on its input and call different link generation functions depending on what it thinks it has been passed?

In truth we would probably need #type 'link' for external links and to introduce #type 'route_link' for internal links so we don't have to guess, but then the Render API has to have near duplicate rendering implementations for the same markup. This is undesirable if it can be avoided as themers don't care about routes vs. paths vs. external links and the extra complexity of seemingly duplicate element types will just cause confusion and misunderstandings.

Unfortunately, I don't know enough about the new route system to answer this question myself, but if we need a "new l()", can it be written to support external URLs as well without causing problems elsewhere? if we can do that it would become relatively easy to re-write #type 'link' to be based on routes instead of l() as part of the general theme system cleanup.

What about keeping things as is, but allowing l() to take an array instead of a string, where:

array == route with parameters
string == old style link (whatever that means)

That would map nicely to what we have now and is compatible with #type.

Also could we get some extra examples, where the needed parameters should / need to be used?

There has been much consensus in the community that keeping l() is a good thing (in different issues related to the theme system), so I can't see the need here and why it is that urgent?

And if all needs to changs, why would l('A Node', 'node/1') or url('node/1') still work at all?

Back to needs review ... All the url related things are unreleated. We could get the link generator in first and then deal with everything done by #2057155: Add a generateFromRoute() method on the url generator to accept options like url()

@thedavidmeister: Let's leave '#type' => 'link' out of this completely, please. I'm aware that it uses l() for internal paths, but that doesn't change the fact that contrib modules should not do that. We will figure out what to do with '#type' => 'link' in a separate issue. Baby steps.

@thedavidmeister: The fact that hook_link_alter() and hook_route_link_alter() (which *is* documented, btw) are similar and modules might end up needing to implement both in the same way is unfornatunate, I completely agree with you. That should not stop the introduction of this hook, however. If we move the remaining use-case of l() (generating a link from an (external) path) onto LinkGenerator as well, we might even be able to consolidate the two hooks. As I already noted above, the calling of these alter hooks is very strange anyway, as it allows to alter less things than you would generally expect. So again, let's please just get this in, and improve this in a follow-up.

I don't know if you're aware of it, but this patch among others is blocking getting the config_translation.module in core, which is a critical feature for Drupal 8. So making progress in baby steps is not just a matter of preference but it is really important in this case. Thanks!

@pwolanin: Thanks for splitting out the UrlGenerator-related parts into a separate issue. Luckily, because LinkGenerator already has $options built-in in the current patch the changes for this issue will be minimal.

I don't know if you're aware of it, but this patch among others is blocking getting the config_translation.module in core, which is a critical feature for Drupal 8. So making progress in baby steps is not just a matter of preference but it is really important in this case. Thanks!

@tstoeckler, I'm not sure if this was your intention, but this did come across as a bit inflammatory. I'm not trying to ruin anyone's dreams or block patches because of ???, I'm allowed to voice my concerns with the quality/implications of a patch regardless of what it is blocking.

Before the recent name change of the issue, this was partially about deprecating/discouraging a function that we were actively increasing the usage of elsewhere. That's a valid concern, I believe.

I am happy to figure out what to do with #type 'link' in a separate issue, as long as that separate issue is followed up soon after this goes in and we're not planning to launch D8 with a key part of the Render API built upon something we're telling contrib modules they should be avoiding. @dawehner doesn't want to wait years for this "to be done right", fair enough, but I don't want to wait years for the ability to represent links as structured data to be re-implemented either if it's destroyed by a poorly executed transition between one method of link generation and another - if there's no compatible render element for the new generator then it is forcing "early render" which is sometimes ok and sometimes really not ok, depending on the context.

So, summary of my points against the most recent patch, even if we aren't deprecating/discouraging/modifying #type 'link':

- Introduces a new alter that is confusing as it splits altering links based on their href, I still don't see why we can't just use 'link' as the alter for both and leave it up to the alter implementations to decide whether they care about routes/paths or if they're just trying to add an attribute to the link. If we're really concerned that the alter won't be able to tell the difference, we could pass in some contextual information to the alter like 'route' => TRUE or something?
- Hasn't been profiled
- Is rendering markup and is only compatible with "early render", which is something we're trying to clean up in #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead and doesn't provide any way to avoid the early render (which would usually be the implementation of a render element type).

The last point I appreciate is probably well beyond the scope of this issue, but I hope others agree that it is something we want to address before D8 is launched and to not handball it to D9.

Oh, one more thing and I'll leave this issue alone for a bit :P

- The docs shouldn't mention l() being deprecated/discouraged or recommend usage of the new link generator until we're confident that migrating directly from l() to the new generator will be possible in most, if not all, cases. At the moment, without being able to handle external links we should be more restrained in our recommendations. There will be times where developers legitimately won't know whether the link they're rendering will have a href that points to an internal or external URL. If we're going with the "baby steps" approach, this applies to the docs too.

Yes, it is great that twig introduced a pattern of a performance test on every issue, but this is not the reality in any other place in the core issue queue.

I am happy to figure out what to do with #type 'link' in a separate issue, as long as that separate issue is followed up soon after this goes in and we're not planning to launch D8 with a key part of the Render API built upon something we're telling contrib modules they should be avoiding. @dawehner doesn't want to wait years for this "to be done right", fair enough, but I don't want to wait years for the ability to represent links as structured data to be re-implemented either if it's destroyed by a poorly executed transition between one method of link generation and another - if there's no compatible render element for the new generator then it is forcing "early render" which is sometimes ok and sometimes really not ok, depending on the context.

Let's do that.

- Introduces a new alter that is confusing as it splits altering links based on their href, I still don't see why we can't just use 'link' as the alter for both and leave it up to the alter implementations to decide whether they care about routes/paths or if they're just trying to add an attribute to the link. If we're really concerned that the alter won't be able to tell the difference, we could pass in some contextual information to the alter like 'route' => TRUE or something?

That is a total valid concern. ... Well, the contextual information is a difference, so having two hooks seems easier to understand.

Hasn't been profiled

Tobias tried, I hope he will manage to do it. There are some issues with drupal.

- Is rendering markup and is only compatible with "early render", which is something we're trying to clean up in #2046881: [meta] Avoid calling drupal_render() "early", wherever it is beneficial to do so, build and return structured data instead and doesn't provide any way to avoid the early render (which would usually be the implementation of a render element type).

It would be great if you could just create a follow up, which describes what you have in mind!

I don't understand the case where you wouldn't know if you have an internal or external link? In the case of a route there would really be no "href", but a route name and parameters.

What you're saying only makes sense if you make a separate #type 'link' and #type 'route_link', but then what if we want to make a #type 'more_link', do we then have to make a #type 'more_route_link'? In #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays there are 11 functions/templates that used to be running through theme(), we've converted almost all of them to #type 'link', but the "next step" currently looks to be #2043649: Make all #type 'link' arrays 100% renderable, use #theme_wrappers if necessary which is to re-implement a bunch of them as their own element types that would be "derivatives" of #type 'link', so then we now need to potentially create 22 new element types to have 'link' and 'route_link' equivalents for everything?

This sounds insane to me, if the new generator could just handle external links, we can rewrite #type 'link' right now and all move forward with our lives.

I did say we can leave that to a follow-up issue though. Let's leave the conversation about avoiding duplicate render element types to that issue.

Yes, it is great that twig introduced a pattern of a performance test on every issue, but this is not the reality in any other place in the core issue queue.

In the past I've been careful to get any changes to l() profiled and optimised because it is called very often #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig and I have no doubts that others have been similarly careful with their changes. If your long term goal is to remove l() and replace it with a link generator, the generator has to be comparably fast - This has nothing to do with Twig, it has to do with the fact that the function you're aiming to replace has a history of being scrutinised for performance implications for at least the last 3 major releases of Drupal.

See D6:

// Remove all HTML and PHP tags from a tooltip. For best performance, we act only
// if a quick strpos() pre-check gave a suspicion (because strip_tags() is expensive).

D7:

// Determine if rendering of the link is to be done with a theme function
// or the inline default. Inline is faster, but if the theme system has been
// loaded and a module or theme implements a preprocess or process function
// or overrides the theme_link() function, then invoke theme(). Preliminary
// benchmarks indicate that invoking theme() can slow down the l() function
// by 20% or more, and that some of the link-heavy Drupal pages spend more
// than 10% of the total page request time in the l() function.

D8:

// Because l() is called very often we statically cache values that require an
// extra function call.

All of the work that I linked in #37 is mostly inspired by a 10-20% speed increase in rendering links through the Render API over the previous implementation.

At one point we even vetoed moving some of the logic in l() into a public function that seemed useful (url_is_active()) because one extra function call in l() was deemed "too slow" after profiling... that's more or less equivalent to the alter added in this patch. So at the very least, based on past profiling of l() I know that the new link generator is doing something we disallowed in l() previously for performance reasons so for that reason alone we should be inlining the logic that's currently in hook_route_link_alter() in the most recent patch.

Please, if "there are some issues with Drupal", that make profiling impossible, a clear report outlining why profiling is impossible would be a hundred times more useful to others who may want to verify or refute that claim than offhand comments about why it's too hard and some parts of the issue queue can ignore performance and other's can't.

I see the comment about l() being discouraged is still in place too with no further discussion...

Marking this needs work because:

• Needs profiling, still has the "Needs profiling" tag, if profiling is impossible we need an explanation of exactly why
• Feedback on how the performance of the patch could be improved has been ignored, despite the lack of profiling
• Documentation still says that l() is "discouraged" despite the issue name being changed to no longer mention this and discussion about "baby steps" and all that...
• I still think the alters should be combined for route_link and link for the sake of DX. The fact that route_name and parameters exist in $variables should be enough context for anyone.

I'm going to have a crack at a re-roll with the last 3 points addressed. I don't want to be the guy who just complains and doesn't help out ;)

I'm going to have a crack at a re-roll with the last 3 points addressed. I don't want to be the guy who just complains and doesn't help out ;)

Thank you very much!

It's vital that this change is profiled since l() is on the critical path for many sites. Let's make sure we don't skimp on it here.

And @dawehner

Yes, it is great that twig introduced a pattern of a performance test on every issue, but this is not the reality in any other place in the core issue queue.

You're correct that it is not reality for many issues, but that does not make it right. For example, it really shouldn't be the twig team that finds issues such as #2051847: AnnotatedClassDiscovery::getDefinitions is critically slow when viewing nodes with comments.

so for that reason alone we should be inlining the logic that's currently in hook_route_link_alter() in the most recent patch.

Completely ignore what I said there. I actually thought that hook_foo_alter() did something, but I tested it and it clearly doesn't unless it's actually a module implementing the hook, I guess I'd never tried it before or really had a reason to look into it, so I was just totally wrong about how this works >.<

Apologies for any confusion caused by my earlier confusion.

Anyway, here's an updated patch that builds on #54 by updating the docs to be clearer about when to use and avoid l() and the generator and merging the alters into just drupal_link_alter() - I think if we can assume #route_name gives us enough context in drupal_pre_render_link() we can probably assume that 'route_name' gives us enough context in drupal_link_alter().

It would be great if you could just create a follow up, which describes what you have in mind!

@dawehner, you've already done this by adding route support to drupal_pre_render_link() in #54! no need for a followup.

What this means is that we can now do this:

$link = array(
  '#type' => 'link',
  '#route_name' => 'foo',
  ...
);
return $link;

Which means $link can be rendered elsewhere/later and therefore is no longer "early", in the context of #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead.

Wow, I missed a lot.

@thedavidmeister: I'm sorry that I sounded inflammatory. I apparently failed to communicate that, but I agree with almost all of your concerns. It was just that the concerns were not actually technical but theoretical and I was afraid that resolving them would greatly broaden the scope of this issue, which is already pretty hard. @dawehner and you now have found a way to (I assume) mitigate your concerns without reworking the patch, at least not in a significant way. So, progress++

Repeating myself from #28, can someone clarify in what way this is a critical issue, and what the compelling use case is that requires it? In absence of that, it might better to have this provided by contrib at this point, because it sounds like it's already confusing the heck out of people about which "link" function to use when and how.

This:

If we get this in now it's going to require a bunch of follow-ups, but maybe that's fine.

...scares the bejeebus out of me. We already have about 50 of these "bunch of follow-ups to implement new API x" issues, most of which are between 0% and 30% done, and all of which will cause Drupal 8 to be harder to learn and approach for new people if they're not all 100% done by release. And given we have over 100 critical issues to sort out before then, I'm not keen to add more things to that list.

update

Added an example why this is helpful. In general we went with using the url generator so why should we stop in the middle of it. Seriously, some things are desperate dis-motivating.

...scares the bejeebus out of me.

LOL.

Beside from that this patch moves things to be lazyloaded as well as introduces a full testcoverage of it, which did not really existed in that detail before.

webchick: We are moving toward "route names everywhere". Right now, if you have a route name, there's no way to create an A tag out of it. That's a critical omission that results in very inconsistent and confusing code.

Saying "well always use route names, except in links where you have to use a path, but here the path has a leading / and over there it doesn't, but that's a route name so you have to do it this other way" is NOT a developer-friendly situation. :-) It is, however, the current situation. This issue is trying to help fix that.

We could also generate the url with the url generator and pass it along to the l function, but don't ask how this code will look like...

Thank you @thedavidmeister for the patch in #2047619-59: Add a link generator service for route-based links

The active class issue seems to need some time and seriously removing the code here will be trivial.
@tstoeckler
Do you have some time to make some benchmarking?

1. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -156,14 +156,19 @@ public function link($text, $route_name, array $parameters = array(), array $opt
        $url = String::checkPlain($this->urlGenerator->generate($variables['route_name'], $variables['parameters'], $variables['options']['absolute']));
   ...
   +    $system_path = $this->urlGenerator->getPathFromRoute($variables['route_name'], $variables['parameters']);
   +    $variables['url_is_active'] = ($system_path == $this->active['system_path'] || (empty($system_path) && $this->active['front_page']))
   

   Just in general it seriously feels wrong to have to call the url generator twice, but as this will be temporary anyway, no deal.

2. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
   @@ -64,7 +64,7 @@ public static function getInfo() {
   +    $this->urlGenerator = $this->getMock('\Drupal\Core\Routing\UrlGenerator', array(), array(), '', FALSE);
   

   Then mock the interface defined by drupal instead.

Disclaimer: I didnt read all the previous comment.

Besides the missing interface, which i already talked about in irc:

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.phpundefined
@@ -0,0 +1,201 @@
+    return '<a href="' . $url . '"' . $attributes . '>' . $text . '</a>';

i dont feel right with this..How about separating the HTML part from the rest of the logic?
Because we effectively make this service useless if one doesnt want to generate HTML.

And by separating the logic, means we could reuse it in l(), because how i see it now, its more or less the same.

eg:
have methods:

• buildFromRoute() (returns a render array, or anything, just not HTML)
• buildFromPath() (same)
• renderHtml() (takes the render array or the anything and makes it HTML)
• link() (calls buildFromRoute or buildFromPath depending on type and then renderHtml)

The interface should have all the methods above besides renderHtml, so you can rely on those being there

#71 sounds similar in concept to #1985974: Make l() optionally return structured output.

There have been attempts to abstract the inner logic of l() into different public functions in the recent past during the Twig conversion and they've all hit walls on the performance gate, but maybe it makes more sense here.

a ha thanks for the link #1985974: Make l() optionally return structured output

yes, what i am saying, is that

a) we duplicate code
b) we mix, logic and alter hook with html
c) i have no way to run the same logic (eg find whether a link is active) if i dont want to generate html

also, no need to change the signature of l()..just make it internally call
return theme('link', \Drupal::service('link_generator')->buildFromPath());

it keeps same arguments and return the same thing..it is true there are more function calls, but in my books, maintainability comes first, not CPU cycles

#75 - heads up that theme() is deprecated. Using '#type' => 'link' inside l() and friends would be great rather than the other way around IMO, but that's a discussion for another day.

#theme => 'link' does not exist.

it keeps same arguments and return the same thing..it is true there are more function calls, but in my books, maintainability comes first, not CPU cycles

@ParisLiakos - if you had read the issue thread you would have seen #54, #55 and #58 where it was made crystal clear that if we are proposing a replacement for l() then it has to perform comparably, or at the least profiled carefully so we know the extent of the damage. The performance gate is an official part of getting patches into core - https://drupal.org/core-gates#performance - "maintainability", while important, is actually not a core gate.

getting the testbots on #78 and #79 so we can see.

well. if we care that much for performance and function calls here then we shouldnt do it as a service, but rather a simple function.
but well, i will now shut up and stop derailing this issue.

What's wrong with ==?

"==" for example has problems with the order.

Fixed a couple of small points and the tests.

I don't think == has problem with the order? We have tests in place for l() having a different order in the query that work fine with ==.

See https://drupal.org/node/1922454#comment-7187362 for discussion + existing tests.

There's no tests for order of query parameters that I could see in #86.

This seems overall sane. Some comments below.

Shouldn't we be turning l() into a dumb wrapper around this service, as we've done elsewhere?

1. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -0,0 +1,160 @@
   +    // Merge in default options.
   +    $variables['options'] += array(
   +      'attributes' => array(),
   +      'query' => array(),
   +      'absolute' => FALSE,
   +      'html' => FALSE,
   +    );
   

   Possible scope creep, but at some point we should change the "html" property as it is horribly misleading. In know in theory it means "this contains HTML, so trust me, really", but it's completely non-descriptive of what is *actually* implied, which is "don't check-plain me, trust me on the content".

   When moving to a new API anyway seems like the logical time to fix that, but I suppose others may disagree.

2. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
   @@ -0,0 +1,313 @@
   +  /**
   +   * Tests the link method.
   +   *
   +   * @see \Drupal\Core\Utility\LinkGenerator::link()
   +   */
   +  public function testLink() {
   +    $this->urlGenerator->expects($this->exactly(7))
   

   This method looks like it really ought to be using @dataProvider instead. If not, it should be broken up into separate methods.

   (Separate methods in PHPUnit are super cheap compared to Simpletest, and are a good practice. A zillion tests in one method is only something we do in Simpletest to side-step its performance design flaws.)

3. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
   @@ -0,0 +1,313 @@
   +  /**
   +   * Tests the active class on the link method.
   +   *
   +   * @see \Drupal\Core\Utility\LinkGenerator::link()
   +   *
   +   * @todo Test that the active class is added on the front page when generating
   +   *   links to the front page when drupal_is_front_page() is converted to a
   +   *   service.
   +   */
   +  public function testLinkActive() {
   +    $this->urlGenerator->expects($this->exactly(6))
   +      ->method('generate')
   +      ->will($this->returnValueMap(array(
   +        array('test_route_1', array(), FALSE, '/test-route-1'),
   +        array('test_route_1', array(), FALSE, '/test-route-1'),
   +        array('test_route_1', array(), FALSE, '/test-route-1'),
   +        array('test_route_1', array(), FALSE, '/test-route-1'),
   +        array('test_route_3', array(), FALSE, '/test-route-3'),
   +        array('test_route_3', array(), FALSE, '/test-route-3'),
   +      )));
   

   Ibid. This definitely feels like it needs a @dataProvider.

Shouldn't we be turning l() into a dumb wrapper around this service, as we've done elsewhere?

That would require converting every l() and #type => 'link' to use routes instead of paths in this patch, wouldn't it?

Possible scope creep, but at some point we should change the "html" property as it is horribly misleading.

yes, scope creep indeed ;) What property name do you propose?

yes, scope creep indeed ;) What property name do you propose?

"escape" with a default to true?

Possible scope creep, but at some point we should change the "html" property as it is horribly misleading. In know in theory it means "this contains HTML, so trust me, really", but it's completely non-descriptive of what is *actually* implied, which is "don't check-plain me, trust me on the content".

Here is a followup: #2070119: Switch from html to escape in the l() function / link generator

Shouldn't we be turning l() into a dumb wrapper around this service, as we've done elsewhere?

That would require converting every l() and #type => 'link' to use routes instead of paths in this patch, wouldn't it?

Yes, we can't do both at the same time.

One possible follow up would be to move the code of l into a linkFromPath method on the linkgenerator, but I would like to also do that on a follow up, as it will cause also more discussion
Follow up: #2070147: Move l() into the linkGenerator

There's no tests for order of query parameters that I could see in #86.

I haven't reworked the testLinkActive method as it is will be removed in the future anyway. We have a basic good test coverage

This method looks like it really ought to be using @dataProvider instead. If not, it should be broken up into separate methods.

Let's do it and also split up the tests into logical groups.

I haven't reworked the testLinkActive method as it is will be removed in the future anyway. We have a basic good test coverage

Ok, I was just going on the assumption that we'd like to have at least equivalent test coverage to l().

Also, I'm concerned that the new active class comparison introduced here is different to l(), has no tests, and looks like a perf hit. Adding the active class server-side may or may not be removed, it hasn't been decided yet #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links.

I tried profiling this. I just a dumb scripts that runs l() and Drupal::linkGenerator->link() a bunch of times in a bootstrapped Drupal. Calling each function 1000 times and then doing that test 100 times I get a 4% performance hit. Calling each function 10000 times I get a 11% performance hit. That's kind of strange, so I don't know if I did something fundamentally wrong or not. Anyway, someone should xhprof this or something.

Hmm, that was unintentional.

hmmm, passing no judgement here, but wanting to provide some context, we removed theme_link() partially because profiling showed it was about 5% slower than calling l() and the original Twig template conversion issue that was proposed for theme_link() was completely blocked on a 10% performance difference between the template and l().

Conversions for Twig are/have been considered "not comparable" if they result in more than about 0.5% difference in perf.

Arguably, this service is more useful than theme_link() was, but those numbers aren't encouraging :( I think I'd like to have a crack at doing some similar basic test runs with timers to see if I can replicate that.

I might try a test that involves different languages, "active-ness" and paths/routes, including absolute URLs and query strings to make it a bit more representative of a page with variation in rendered links?

Yeah, in case that didn't come across, I wouldn't really advise anyone to trust my numbers.

class comparison introduced here is different to l(), has no tests, and looks like a perf hit. A

Just in case someone stumbles upon that: Also the active part has a test coverage.

We have to figure out where this performance problem might be. If it is loading of the route objects, maybe #2058845: Pre-load non-admin routes would help in reality.

+    $variables['url_is_active'] = $route_name == $this->active['route_name']
+      && (count($full_parameters) == count($this->active['parameters']) && !array_diff_assoc($full_parameters, $this->active['parameters']))
+      // The language of an active link is equal to the current language.
+      // @todo - the language will not actually be reflected in the output
+      //   until the new generator method in https://drupal.org/node/2057155 is
+      //   available and used.
+      && (empty($variables['options']['language']) || $variables['options']['language']->id == $this->active['language']);
+

This changes the behaviour of active class generation from l() in two ways:

- The order of checking conditions has moved query checking above language, this is a perf hit for sites making heavy use of languages
- == has been replaced with a more complicated method of checking equality

There were no failing tests demonstrating a problem before either of these changes were made.

There is no test coverage in the patch equivalent to the following test (in UrlTest.php) that already exists for l():

    $links = $this->xpath('//a[@href = :href and contains(@class, :class)]', array(':href' => url($path, $options_query_reverse), ':class' => 'active'));
    $this->assertTrue(isset($links[0]), 'A link generated by l() to the current page with a query string that has matching parameters to the current query string but in a different order is marked active.');

What I'm saying is, there's no proof that we need to replace == with (count($full_parameters) == count($this->active['parameters']) && !array_diff_assoc($full_parameters, $this->active['parameters']), the only reason given is that == is sensitive to order, but the test that already exists for l() shows it isn't, and I'm concerned that the latter is a perf hit.

The PHP docs say that == just checks for key/value pairs and only === check for order - http://php.net/manual/en/language.operators.array.php

As @dawehner said in #100, we have to figure out where the perf hit alluded to in #96 is actually coming from before we can discuss it meaningfully. It's important to know whether some part of the patch is causing a slow-down and needs fixing or if it's the route system itself being slower than the path system - in which case there'd not really be anything further we could do here.

What I was talking about in #101 would have nothing to do with #96 as the benchmarking there had no language or query active URL checking in the test, but thanks for reigning in the scope creep a bit @pwolanin :)

Rerolled against latest core.

The third parameters should be just the options. Ensure that we have test coverage for that as well now.

There is already a follow up filled for that: #2051813: Reuse the link generator to provide a Drupal::link() function

Let me readd a test for passing in query via the $parameters.

I am comfortable with this.

still needs profiling

The issue is still assigned to you :p

woah, that's from #56

A comparison to l() alone isn't exactly meaningful since this will have to load the route from the DB (or cache?) if it's not already loaded.

It's meaningful because in this patch we're still recommending in the docs for l() that developers use this link generator instead of l() without explaining (or even knowing) the performance consequences of using one over the other (In the D7 docs for l() we were careful to explain to developers the performance consequences of making changes to the usage of l()).

It's meaningful because the consumers of Drupal, end-users, site owners, developers, do not care so much about the "why" if something that was previously fast and *should* be fast suddenly becomes slow. The question here is very simple, "how long does it take to render a link, using the standard core mechanlsm?".

It's meaningful because contributors have suggested alternative implementations to a service that may potentially be faster if a signifiant perf hit is identified, including an rl() procedural function or adding route support directly to l().

It's very obvious from this issue thread that getting community approval for this patch is not just about the direct impact of this patch, but the implementation has to function as a starting point for a long term goal to completely replace l() throughout core and contrib with whatever is implemented here. There's no point pretending that this is a simple "commit and move on" situation just because we have a good quality, green patch and RTBC is in sight.

If the profiling comes back showing a significant slowdown, this would impact what recommendations we make and how far we should take the service. Do we just use this to unblock other critical issues that directly rely on this patch and revisit the problem in D9, or do we immediately push for all of core to use this service before launch? @webchick has already voiced concerns in #62 over "what happens next?" here, the potential perf hit of using routes instead of l() will surely temper how enthusiastically we push any future conversion issues. How slow would be "too slow"? 10%? 50%? "too slow" meaning we would have to start re-writing things here even if we have to sacrifice DX, or even postpone this issue on other issues that make route lookups faster such as #2058845: Pre-load non-admin routes linked in #100?

The core gate docs for performance outline the reason to profile a patch for CPU with XHProf:

If a patch adds or changes any code that is run during a 'normal' request to Drupal, especially up to and including full bootstrap, or during the page rendering process

That is an excellent description of exactly what we're doing here. If we absolutely can't use XHProf, #96 shows we can still get some basic benchmarks without it that may suffice.

In #58 @alexpott said this:

It's vital that this change is profiled since l() is on the critical path for many sites. Let's make sure we don't skimp on it here.

So, if you don't want to profile, please don't just say "IMO we don't need to" and hit RTBC but clearly explain how/why this patch is exempt from passing the core gates, and why we should be ignoring what @alexpott has already said about comparing the performance of a route-based service to l().

I definitely didn't make such a commitment to using routes everywhere in D8 (not sure if Alex/catch/Dries did, or where it was publicly stated), especially given how late we are in the cycle and how much WSCCI is contributing to the overall release blockers ATM. There is certainly agreement among core maintainers to not ship D8 with two routing systems, but that just means finishing #1971384: [META] Convert page callbacks to controllers, which is already critical and in-progress.

If committing this patch does indeed set us on a path to oblierate l() usage throughout core, profiling is a must-have prior to commit. There was already extensive work done by the Twig team in order to make l() nice and speedy; we need to make sure that work is reflected in the "new world order." The last thing we need is yet another vector where we need to try and claw back performance.

There are plenty of places in code where we'll have a route name but no path. In order to get an A tag out of that, you'll need to call the generator with that route to get a path, then use that path (give or take some string mangling for leading slashes) with l().

This service is just wrapping up that process to make it easier, as I understand it. The same amount of work / same performance impact or lack thereof will be present either way. We can either make that process easy, or make it hard/obtuse. I'd rather make it easy.

Whether we commit to removing all traces of paths from core or not is irrelevant. We will need to go from route name to A tag at times, and this service makes the process of doing so easier.

Let me try to make a point that this improves the DX:

Some example code to provide a link with a path coming from the routing system.

$url = Drupal::urlGenerator()->generate('views_ui.edit', array('view' => 'frontpage'));
$path = drupal_substr($url, 0, strpos(base_path(), $url) + 1);
print l(t('Text'), $path);

After the patch

print Drupal::linkGenerator()->generate|link|whatever(t('Text'), 'views_ui.edit', array('view' => 'frontpage'));

Changed the patch to use generate() as it is a verb in contrasts to link and it makes it more parallel with the way how UrlGeneratorInterface looks like.

missing interdiff

1. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Controller/ViewsUIController.php
   @@ -20,7 +20,8 @@
   -use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
   +use Drupal\Core\Routing\UrlGeneratorInterface;
   
   @@ -44,24 +45,32 @@ class ViewsUIController implements ControllerInterface {
   -   * @var \Symfony\Component\Routing\Generator\UrlGeneratorInterface
   +   * @var \Drupal\Core\Routing\UrlGeneratorInterface
   ...
   -   * @param \Symfony\Component\Routing\Generator\UrlGeneratorInterface
   +   * @param \Drupal\Core\Routing\UrlGeneratorInterface
   ...
   +  public function __construct(EntityManager $entity_manager, ViewsData $views_data, UrlGeneratorInterface $url_generator, LinkGeneratorInterface $link_generator) {
   

   I really think this is just wrong. This couples the code a bit more drupal for no win at all, but I won't fight this.

2. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Controller/ViewsUIController.php
   @@ -44,24 +45,32 @@ class ViewsUIController implements ControllerInterface {
        $this->viewsData = $views_data;
        $this->urlGenerator = $url_generator;
   +    $this->urlGenerator = $link_generator;
   

   You seem to have not tested the change ... you override the urlGenerator with the linkGenerator :)

re: the UrlGeneratorInterface - we might e.g. prefer to use the options array, so I think any Drupal module code should use the Drupal-provided interface (that was recently committed).

Well, but we don't need it here yet. Let's not fight here.

Opened an issue to write tests for this routes: #2073779: Add tests for admin/reports/fields/views-fields and admin/reports/fields/views-plugins

• Adds a method on the controller base for l()
• Did not added a method on the formbase as I could not find a form using l(). (i went through 80% of them already)
• Used the l() method in the dblog controller

Some followups: #2073811: Add a url generator twig extension, #2073813: Add a UrlGenerator helper to FormBase and ControllerBase

Changes from today look like a big improvement to me.

+1 for these changes.

May I propose an approach to getting the performance stuff ticked off so we can move forward? The important question here isn't whether routes or paths or absolute URLs are faster, or even what regressions are "introduced by this patch" and what is "external" to it, but simply:

How long does it take to render a link in Drupal the "right" way before and after the patch lands?

Using the microtime() approach from #96 with X runs we could get a minimum and some middle ground, average + std dev? median? for:

1. l()
2. l() with an inline URL-from-route mechanism
3. A link generated by whatever the latest patch here implements when we test

for:

A. An internal path/route
B. An internal path/route with a language
C. An internal path/route with a query
D. <front>

If 1, 2 and 3 are all comparable (which would be no more than a 1-2% regression from this patch vs. l()) then great, we're done here, RTBC! :)

if 2 is "too slow" relative to 1, then we have an issue with the routing system itself being too slow and should postpone this issue on critical performance improvements to the route system that are likely to make up the difference.

if 3 is "too slow" relative to 1 and 2 then we have more work to do on the code in this patch and the route system can be left alone for now.

That's reasonable, yeah?

Here is a very naive benchmark code:

    for ($i = 0; $i < 10000; $i++) {
      l('Edit view', 'admin/structure/views/view/frontpage');
    }
vs.
//    $link_generator = \Drupal::linkGenerator();
//
//    for ($i = 0; $i < 10000; $i++) {
//      $link_generator->generate('Edit view', 'views_ui.edit', array('view' => 'frontpage'));
//    }

We could really trick a LOT in the benchmarks by using a cached url generator (which happens in another issue).

Here is a benchmark which does not test the link generator but shows the potential we have when we use routes
instead of paths for menu links.

$map = array();
    for ($i = 1; $i <= 1000; $i++) {
//      menu_item_route_access($route, "taxonomy/term/$i/delete", $map);
       $access_manager->checkNamedRoute('taxonomy_term_delete', array('taxonomy_term' => $i));
    }
    return 'Foo';

Yes it is unfair, because menu_item_route_access() triggers a DB call, but checkNamedRoute does not, but there is also way less php code to be called.

yay!

added some explanation

I'm still comfortable.

I feel like catch or Alex would be much better candidates to review/commit this than me, given the earlier performance concerns and my 3 month hiatus where I missed most of the stuff leading up to this patch. Not sure which is best to assign it to, so just leaving it for one of them.

@webchick, the comments in this thread are a poor record of the conversation we had in IRC, apologies.

We specifically discussed what we would want to convert and what we wouldn't in context of the "yet another conversion" problem. It's a concern you raised in #62 and wasn't really addressed properly until now.

In IRC we discussed planning to convert anything with an existing access check to the generator as the profiling shows the generator is much faster than l() when access checks are involved. This was added to the issue summary but it's not hugely prominent.

We'd hold off converting anything else until we can get the doGenerate() optimised further and maybe working on the route system more, to the point where profiling brings the generator more inline with l() everywhere.

Does this make sense to you?

Alex feels that catch's feedback here would be helpful, so assigning it to him.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -0,0 +1,161 @@
+    // Because this service is called very often we statically cache values that

Was this profiled?

Won't it break if the request object gets messed around with?

Why would we have multiple instances of the link generator knocking around, I'd expect it to be instantiated once from the container and that's it?

Fixed that piece of documentation.

I think catch is just talking about

// Because this service is called very often we statically cache values that

being profiled, but this has not been profiled for this issue.

That text/logic was copied and pasted from l(), and introduced in #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig. It may not make sense in this context.

I got confused by 'statically cache' when they're just normal class properties, couldn't see any reason at all why they'd need to be static class properties when they're not. Storing them as separate properties is good, removing the request property even better since then it's clear that's all we need.

bumping it back to RTBC then since everyone seems happy with the addressed conern.

OK. Committed/pushed to 8.x.

Needs a change notification for the API addition. I have a feeling there's an issues somewhere about caching generator stuff a bit more, but if there's no an actual dedicated issue for it, let's open one before closing this out - this is exactly the feature that if people use it will result in a lot more generator calls.

Thank you very much. https://drupal.org/node/2078263

Sigh. #2078287: [meta] Fix the DX of the link generator

From #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays, pwolanin:

based on #2047619: Add a link generator service for route-based links , we should start using #route_name and #route parameters instead of #href where possible

What is going on here?

I was pretty sure that we all agreed we'd only be doing #route_name conversions for links that currently have access checks, until the performance improvements for the generator land, at which point we'd look at conversion with a wider scope.

Less than 24 hours after this patch is committed and we're already on the "convert everything" train?

What changed between now and #146? Why are we abandoning the plan outlined in the issue summary?

Follow-up: #2091901: Wrong one-line description on UrlGenerator and LinkGeneratorInterface

Updated issue summary.