Switch Drupal::l() and LinkGenerator to expect a Url object

Modified conflicts in the following files.
CONFLICT (content): Merge conflict in core/themes/seven/seven.theme
CONFLICT (content): Merge conflict in core/modules/menu_ui/src/MenuForm.php
CONFLICT (modify/delete): core/modules/menu_link/menu_link.module
CONFLICT (modify/delete): core/modules/contact/src/CategoryForm.php
CONFLICT (content): Merge conflict in core/modules/comment/src/CommentBreadcrumbBuilder.php
CONFLICT (content): Merge conflict in core/modules/comment/src/Controller/AdminController.php
CONFLICT (content): Merge conflict in core/modules/block_content/src/BlockContentTypeForm.php
CONFLICT (content): Merge conflict in core/modules/block_content/block_content.pages.inc
CONFLICT (content): Merge conflict in core/includes/menu.inc
CONFLICT (content): Merge conflict in core/includes/common.inc

Deleted files: core/modules/contact/src/CategoryForm.php
core/modules/menu_link/menu_link.module

Modified core/includes/menu.inc the function in conflict had been moved to core/lib/Drupal/Core/Render/Element/Link.php
The method name is preRenderLink

We discussed in #2343651: Remove most remaining l() calls the impact this switch would have on links to entities, which currently have a couple ugly flavors in that issue. When you have the full entity object:

+    $link = \Drupal::linkGenerator()->generateFromUrl($this->t('Edit'), $term->urlInfo('edit-form'));
...
-        $this->logger('taxonomy')->notice('Created new term %term.', array('%term' => $term->getName(), 'link' => l($this->t('Edit'), 'taxonomy/term/' . $term->id() . '/edit')));
+        $this->logger('taxonomy')->notice('Created new term %term.', array('%term' => $term->getName(), 'link' => $link));

When you don't:

-        return l($text, 'taxonomy/term/'. $tid . '/edit', array('query' => drupal_get_destination()));
+        return \Drupal::l($text, 'entity.taxonomy.edit_form', ['taxonomy_term' => $tid], array('query' => drupal_get_destination()));

Switching l() to take a Url object would definitely cut down the verbosity for entities (needing to chain the link generator and the generation method) and bring entity links more in line with (then other uses of) \Drupal::l() generally, but what about the extra tedium of generating your Url object all the time?

Also, we'd need to consider the DX of having \Drupal::l() and \Drupal::url() be parallel and do the "same thing" for URL creation, vs. taking different types of arguments.

Seems valid.

but what about the extra tedium of generating your Url object all the time?

IMHO that's a non-issue, because going from

\Drupal::l('text', 'route.name', ['param' => 'value'])

to:

\Drupal::l('text', new Url('route.name', ['param' => 'value']))

really isn't painful; it actually shows clearly that those two parameters that used to be there are really a pair (some programming languages allow to pass pairs of values!); by encapsulating them in a Url object, which is not painful at all, as the example demonstrates, just makes that more explicit.

Also, we'd need to consider the DX of having \Drupal::l() and \Drupal::url() be parallel and do the "same thing" for URL creation, vs. taking different types of arguments.

+1. The question is: do we address that here or in a follow-up?

but I think Drupal::l() is becoming too useful to switch it to take Url.

Is it really that bad to change

\Drupal::l(t('Front page'), '<front>')

to:

\Drupal::l(t('Front page'), new Url('<front>'))

IMHO that's okay. Same argument as the one at the beginning of this comment.

+++ b/core/lib/Drupal/Core/Routing/LinkGeneratorTrait.php
@@ -41,6 +42,21 @@ protected function l($text, $route_name, array $parameters = array(), array $opt
+   * Renders a link to a URL.

The other way around :P

IMHO that's okay. Same argument as the one at the beginning of this comment.

At least the link generator itself should support both, it is written to support different usecases anyway. I would kind of prefer the first one,
because it adds less coupling between the various systems.

+1. The question is: do we address that here or in a follow-up?

Well, this is quite some API change in that case. A lot of contrib projects might break again here. On other point is that people
might start to use Url::createFromPath() which then would be horrible again.

Bumping to beta-blocking based on #2339219: [meta] Finalize URL generation API (naming, docs, deprecation).

We (effulentsia, xjm, pwolanin and I) discussed this today at length as part of #2339219, and we concluded this: #2339219-55: [meta] Finalize URL generation API (naming, docs, deprecation) (please go and read that comment).

From that conclusion, the following is what applies to this issue:

1. there should be only one to generate links, and as this issue says: since the introduction of the Url object, we now have more code using the object than the route names directly, so the link generator should only generate links using a Url object
2. \Drupal::l() should also accept a Url object, not the route name, parameters, options trifecta, for consistency, and it's then only a procedural alias for the only way to call LinkGenerator, which is conceptually very easy to understand

Also checked with dawehner, and he can live with this also.

This patch does not build on top of previous patches in this issue. Drupal installs, all unit tests pass, so I'm fairly hopeful this one will be green :)

1. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkInterface.php
   +++ b/core/lib/Drupal/Core/Menu/ContextualLinkInterface.php
   @@ -50,9 +50,7 @@ public function getGroup();
   
   @@ -50,9 +50,7 @@ public function getGroup();
       * Returns the link options passed to the link generator.
       *
       * @return array
   -   *   The options as expected by LinkGeneratorInterface::generate()
   -   *
   -   * @see \Drupal\Core\Utility\LinkGeneratorInterface::generate()
   +   *   An associative array of options.
       */
      public function getOptions();
   
   +++ b/core/lib/Drupal/Core/Menu/LocalActionInterface.php
   @@ -30,8 +30,6 @@ public function getRouteName();
   -   *
   -   * @see \Drupal\Core\Utility\LinkGeneratorInterface::generate()
       */
   
   @@ -50,8 +48,6 @@ public function getWeight();
       *   An associative array of options.
   -   *
   -   * @see \Drupal\Core\Utility\LinkGeneratorInterface::generate()
       */
   
   +++ b/core/lib/Drupal/Core/Menu/LocalTaskInterface.php
   @@ -41,8 +41,6 @@ public function getTitle();
       *   An array of parameter names and values.
   -   *
   -   * @see \Drupal\Core\Utility\LinkGeneratorInterface::generate()
       */
   
   @@ -61,9 +59,7 @@ public function getWeight();
       * @return array
   -   *   An array of options.
   -   *
   -   * @see \Drupal\Core\Utility\LinkGeneratorInterface::generate()
   +   *   An associative array of options.
       */
   

   So we basically drop that helpful information what options could be about?

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
   @@ -110,6 +110,7 @@ public function isDeletable();
       * @return string
   +   *   The name of the route this menu link links to.
       */
      public function getRouteName();
    
   @@ -117,6 +118,7 @@ public function getRouteName();
   
   @@ -117,6 +118,7 @@ public function getRouteName();
       * Returns the route parameters, if available.
       *
       * @return array
   +   *   An array of parameter names and values.
       */
      public function getRouteParameters();
    
   @@ -136,7 +138,7 @@ public function getUrlObject($title_attribute = TRUE);
   
   @@ -136,7 +138,7 @@ public function getUrlObject($title_attribute = TRUE);
       * Returns the options for this link.
       *
       * @return array
   -   *   The options for the menu link.
   +   *   An associative array of options.
       */
      public function getOptions();
   

   I mean really, those could be jumped over

3. +++ b/core/lib/Drupal/Core/Render/Element/Link.php
   @@ -83,7 +84,7 @@ public static function preRenderLink($element) {
   -      $element['#markup'] = \Drupal::linkGenerator()->generate($element['#title'], $element['#route_name'], $element['#route_parameters'], $element['#options']);
   +      $element['#markup'] = \Drupal::linkGenerator()->generateFromUrl($element['#title'], new Url($element['#route_name'], $element['#route_parameters'], $element['#options']));
   

   But this is \Drupal::l() right?

4. +++ b/core/modules/node/node.module
   @@ -205,7 +205,7 @@ function node_title_list(StatementInterface $result, $title = NULL) {
   -    $items[] = \Drupal::linkGenerator()->generate($row->title, 'entity.node.canonical', ['node' => $row->nid], $options);
   +    $items[] = \Drupal::linkGenerator()->generateFromUrl($row->title, new Url('entity.node.canonical', ['node' => $row->nid], $options));
   

   \Drupal::l()

5. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
   @@ -353,49 +369,34 @@ public function testGenerateActive() {
   -    // Render a link with the same path as the current path, but with the
   -    // set_active_class option disabled.
   ...
   +    // Render a link with the set_active_class option disabled.
   

   Meh, you could have tried to make the diff smaller by not changing that.

6. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
   @@ -422,35 +420,13 @@ public function testGenerateActive() {
   -    // Render a link with the same path but a different query parameter than the
   -    // current path.
   -    $result = $this->linkGenerator->generate(
   -      'Test',
   -      'test_route_3',
   -      array(),
   -      array(
   -        'query' => array('value' => 'example_2'),
   -        'set_active_class' => TRUE,
   -      )
   -    );
   -    $this->assertTag(array(
   -      'tag' => 'a',
   -      'attributes' => array(
   -        'data-drupal-link-system-path' => 'test-route-3',
   -        'data-drupal-link-query' => 'regexp:/.*value.*example_2.*/',
   -      ),
   -    ), $result);
   -
   -    // Render a link with the same path and query parameter as the current path.
   -    $result = $this->linkGenerator->generate(
   -      'Test',
   -      'test_route_4',
   -      array('object' => '1'),
   -      array(
   -        'query' => array('value' => 'example_1'),
   -        'set_active_class' => TRUE,
   -      )
   -    );
   +    // Render a link with route parameters and a query parameter.
   +    $url = new Url('test_route_4', array('object' => '1'), array(
   +      'query' => array('value' => 'example_1'),
   +      'set_active_class' => TRUE,
   +    ));
   +    $url->setUrlGenerator($this->urlGenerator);
   +    $result = $this->linkGenerator->generateFromUrl('Test', $url);
        $this->assertTag(array(
   

   Did you really killed some test coverage? Just curious.

Discussed with dawehner in person - since the doc changes are done, maybe leave them. Converted 2 more calls to Drupal::l() and fixed a bunch of missing use statements causing fatal.

Let me try to figure out the remaining failures.

Give Wim some sleep, seriously, @random guy out there, don't burn your people. This is horrible if you want to release this Drupal 8 thing.

.

• #22.1, #22.2: I made it consistent.
• #22.3, #22.4: yep, you're right
• #22.5, #22.6: I had to update the comments for the test coverage to make it understandable. #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links introduced this test coverage, but then #2184649: Remove LinkGenerator::getActive() and LinkGenerator::setRequest() came and removed all the setRequest() calls. Without them, the comments no longer made sense. It took me a long time to figure out why the hell this was testing seemingly identical things multiple times… until I realized that's exactly what it was doing: it was testing the same thing multiple times. Hence I just removed the duplicates and updated the comments to be correct.

I think dawehner and I had some miscommunication in git - some use statements went missing again in his last patch.

Let's see if this is working.

/me takes a look

or not

this is basically RTBC, but there is a small conflict with #2343759: Provide an API function to replace url()/l() for external urls, so let's put that in first and then fix this and get it in.

Unpostponing. Neither blocks the other. :)

I reviewed all interdiffs since my patch in #19.

• #38 restored LinkGenerator::generateFromLink
• +1 for s/LinkGenerator::generateFromUrl()/LinkGenerator::generate()/
• Replaced all remaining \Drupal::linkGenerator()->generate() calls with \Drupal::l() calls, for consistency. (At dawehner's request :)) These are trivial changes.

Sadly the #38 interdiff was incomplete, it doesn't list the $this->l() changes. So I had to review the entire 90K patch after all :P

While doing that, @dawehner discovered that LinkGenerator::generate() doesn't call setUrlGenerator() on the passed in Url object. It should, because it avoids three method calls Url::toString() call… which can be quite a lot. The optimization simply didn't exist on LinkGenerator::generateFromUrl() before (probably because it was undocumented). It was approved by both pwolanin and myself.

Since there are only simple changes and we really need to get this done: assuming it comes back green: RTBC.

Committed 4f49087 and pushed to 8.0.x. Thanks!

HEAD is broken on standard install and install with drush with standard install.

Here's a trace when going to core/install.php on a fresh git clone.

( ! ) Fatal error: Cannot use Drupal\Core\Url as Url because the name is already in use in /var/www/drupal8.dev/core/lib/Drupal/Core/Render/Element/Link.php on line 10
Call Stack
#	Time	Memory	Function	Location
1	0.0001	234896	{main}( )	../install.php:0
2	0.0061	971656	install_drupal( )	../install.php:32
3	0.5213	15085760	install_run_tasks( )	../install.core.inc:102
4	0.5218	15119280	install_run_task( )	../install.core.inc:482
5	0.5218	15119560	install_select_language( )	../install.core.inc:588
6	0.5218	15120320	install_get_form( )	../install.core.inc:1195
7	0.5334	16120232	Drupal\Core\Form\FormBuilder->buildForm( )	../install.core.inc:815
8	0.5361	16312744	Drupal\Core\Form\FormBuilder->prepareForm( )	../FormBuilder.php:225
9	0.5368	16367240	Drupal\Core\Form\FormBuilder->getElementInfo( )	../FormBuilder.php:597
10	0.5368	16367272	element_info( )	../FormBuilder.php:1094
11	0.5379	16371240	Drupal\Core\Render\ElementInfoManager->getInfo( )	../common.inc:3218
12	0.5379	16371344	Drupal\Core\Render\ElementInfoManager->buildInfo( )	../ElementInfoManager.php:57
13	0.5815	18124904	Drupal\Core\Render\ElementInfoManager->createInstance( )	../ElementInfoManager.php:73
14	0.5815	18125040	Drupal\Component\Plugin\PluginManagerBase->createInstance( )	../ElementInfoManager.php:99
15	0.5815	18125040	Drupal\Core\Plugin\Factory\ContainerFactory->createInstance( )	../PluginManagerBase.php:71
16	0.5815	18125040	Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass( )	../ContainerFactory.php:21
17	0.5815	18125088	class_exists ( )	../DefaultFactory.php:86
18	0.5815	18125432	Composer\Autoload\ClassLoader->loadClass( )	../DefaultFactory.php:0
19	0.5816	18125568	Composer\Autoload\includeFile( )	../ClassLoader.php:274

The problem is we have other objects called Url in different namespaces - Drupal\views\Plugin\views\field, Drupal\Core\Render\Element

Not sure why this was not caught earlier.

I applied the patch and was able to load the installation screen manually. Waiting on report from testbot_ci which also reproduced the issue with standard install profile using Drush.

Testing the patch 44 using the new testbot infrastructure using the drush installer. The installation worked as expected.

Great catch.

Committed and pushed to 8.x. Thanks!

Found and updated this change record: https://www.drupal.org/node/2189619 ("Use the \Drupal\Core\Url object in place of arrays of route info").

IS API changes might need an update as well.