Replace all calls to the deprecated Drupal::l() function in Core

+++ /dev/null
@@ -1,61 +0,0 @@
-
-namespace Drupal\Component\Plugin;
-
-use Drupal\Component\Utility\NestedArray;
-
-/**
- * Implements \Drupal\Component\Plugin\ConfigurableInterface.
- *
- * In order for configurable plugins to maintain their configuration, the
- * default configuration must be merged into any explicitly defined
- * configuration. This trait provides the appropriate getters and setters to
- * handle this logic, removing the need for excess boilerplate.
- *
- * @ingroup Plugin
- *
- * @todo Add protected $configuration property when PHP 5 is no longer
- *   supported. See https://www.drupal.org/project/drupal/issues/3029004.
- */
-trait ConfigurableTrait {
-
-  /**
-   * Gets this plugin's configuration.

+++ b/core/lib/Drupal/Core/Action/ConfigurableActionBase.php
@@ -6,7 +6,6 @@
 use Drupal\Component\Plugin\DependentPluginInterface;

The patch seems to contain a lot of unrelated changes, diff/rebased against wrong/old branch?

I tried to extract the best parts from #2, plus some reroll as code has changed already.

Rerroll patch to apply and change Link::fromTextAndUrl('text', $url) to \Drupal::service('link_generator')->generate('text', $url).

Rerroll again because last patch was created against wrong commit.

Rerroll patch because replacement of deprecated function was wrong.

Fix service link generator call.

Rerrol patch to pass tests.
Not allways is posible to use \Drupal::service('link_generator')->generate

Rerroll to fix some additional errors.

Fix coding standards Unused use statement Link class.

Change issue status.

> Rerroll patch to apply and change Link::fromTextAndUrl('text', $url) to \Drupal::service('link_generator')->generate('text', $url).

Why? Link::fromTextAndUrl() is not deprecated, that is the recommended replacement?

Tested on Drupal 8.8.x and there is no l() on core files except on test files.

Why? Link::fromTextAndUrl() is not deprecated, that is the recommended replacement?

It is because seeing the class Link https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Link.php/... The method to string calls directly to the link generator. So I do not see the advantage of using the Link::fromTextAndUrl() in the most cases.

  /**
   * Generates the HTML for this Link object.
   *
   * Do not use this method to render a link in an HTML context. In an HTML
   * context, self::toRenderable() should be used so that render cache
   * information is maintained. However, there might be use cases such as tests
   * and non-HTML contexts where calling this method directly makes sense.
   *
   * @return \Drupal\Core\GeneratedLink
   *   The link HTML markup.
   *
   * @see \Drupal\Core\Link::toRenderable()
   */
  public function toString() {
    return $this
      ->getLinkGenerator()
      ->generateFromLink($this);
  }

The Link class is the designed replacement of \Drupal::l(). at least for all the places that can't (easily) be injected. We also very recently removed all usages of the deprecated LinkGeneratorTrait, which I think you rely on in a lot of places, that won't work anymore now.

Comment #20 sounded a bit harsh, wasn't meant like that sorry. I still think we should stick with Link, that's what we did elsewhere, but lets get a second opinion on that.

This is tricky, because Link::fromTextAndUrl() returns a Link object, while \Drupal::l() returns a GeneratedLink object, so trying to get all the right ->toStrings() and ->toRenderables() is tough. Here's a first attempt, that will need iteration. No interdiff because I started from scratch using Link instead of the link generator.

:-/

So it seems I was a little confused. I had thought that Link::toString() was returning a, you know, string. I didn't realize that that was a generated link, so in most cases here, ->toString() is correct. Lets see if this gets us closer.

Well, that went the wrong direction. :-/

1. +++ b/core/lib/Drupal.php
   @@ -609,14 +609,16 @@ public static function linkGenerator() {
       *
   -   * @deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0.
   -   *   Use \Drupal\Core\Link instead.
   +   * @deprecated in drupal:8.0.0 and is removed from drupal:9.0.0. Use
   +   * \Drupal\Core\Link instead.
   +   * @see https://www.drupal.org/node/2614344
       *   Example:
       *   @code
       *     $link = Link::fromTextAndUrl($text, $url);
       *   @endcode
       */
   

   afaik there should be an empty line between @deprecated and @see. And @see is usually last in the docblock. Not sure about the code example here. I think we could just explicitly say Use ...Link::fromTextAndUrl() in the message and drop that, it's really not that much longer.

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -952,7 +953,7 @@ function hook_requirements($phase) {
      $requirements['php'] = [
        'title' => t('PHP'),
   -    'value' => ($phase == 'runtime') ? \Drupal::l(phpversion(), new Url('system.php')) : phpversion(),
   +    'value' => ($phase == 'runtime') ? Link::fromTextAndUrl(phpversion(), new Url('system.php'))->toString() : phpversion(),
   

   I think the recommended way would be Url::fromRoute(), not sure if we want to touch that here. Looks like there are quite a few examples like that.

Addressing feedback, fixing last test.

Some comments/notes, only 1-2 are actionable.

1. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -5,6 +5,7 @@
    use Drupal\entity_test\Entity\EntityTest;
   
   @@ -342,8 +343,8 @@ public function testLinkTitle() {
        $output = $this->renderTestEntity($id);
   -    $expected_link = (string) \Drupal::l($value, Url::fromUri($value));
   -    $this->assertContains($expected_link, $output);
   +    $expected_link = Link::fromTextAndUrl($value, Url::fromUri($value))->toString();
   +    $this->assertContains($expected_link->getGeneratedLink(), $output);
    
   
   @@ -354,8 +355,8 @@ public function testLinkTitle() {
        $this->assertText(t('entity_test @id has been updated.', ['@id' => $id]));
    
        $output = $this->renderTestEntity($id);
   -    $expected_link = (string) \Drupal::l($title, Url::fromUri($value));
   -    $this->assertContains($expected_link, $output);
   +    $expected_link = Link::fromTextAndUrl($title, Url::fromUri($value))->toString();
   +    $this->assertContains($expected_link->getGeneratedLink(), $output);
   

   Hm, not sure if you went through multiple changes here, but wouldn't it be easier to keep the (string) cast? Drupal:l() and toString() actually have the exact same return value, so doesn't seem necessary to change that here?

2. +++ b/core/modules/locale/locale.module
   @@ -647,16 +648,14 @@ function locale_form_language_admin_overview_form_alter(&$form, FormStateInterfa
        if (!$language->isLocked() && locale_is_translatable($langcode)) {
   -      $form['languages'][$langcode]['locale_statistics'] = [
   -        '#markup' => \Drupal::l(
   -          t('@translated/@total (@ratio%)', [
   -            '@translated' => $stats[$langcode]['translated'],
   -            '@total' => $total_strings,
   -            '@ratio' => $stats[$langcode]['ratio'],
   -          ]),
   -          new Url('locale.translate_page', [], ['query' => ['langcode' => $langcode]])
   -        ),
   -      ];
   +      $form['languages'][$langcode]['locale_statistics'] = Link::fromTextAndUrl(
   +        t('@translated/@total (@ratio%)', [
   +          '@translated' => $stats[$langcode]['translated'],
   +          '@total' => $total_strings,
   +          '@ratio' => $stats[$langcode]['ratio'],
   +        ]),
   +        Url::fromRoute('locale.translate_page', [], ['query' => ['langcode' => $langcode]])
   +      )->toRenderable();
        }
        else {
   

   We're changing the form structure here, could theoretically go for a smaller change but it does seem nicer like this.

3. +++ b/core/modules/system/tests/src/Functional/Common/UrlTest.php
   @@ -27,12 +28,12 @@ class UrlTest extends BrowserTestBase {
      public function testLinkXSS() {
   -    // Test \Drupal::l().
   +    // Test Link Generator.
        $text = $this->randomMachineName();
        $path = "<SCRIPT>alert('XSS')</SCRIPT>";
        $encoded_path = "3CSCRIPT%3Ealert%28%27XSS%27%29%3C/SCRIPT%3E";
    
   -    $link = \Drupal::l($text, Url::fromUserInput('/' . $path));
   +    $link = Link::fromTextAndUrl($text, Url::fromUserInput('/' . $path))->toString();
   

   As a non native speaker, all uppercase here seems a bit strange? Below we we just say Test \Drupal\Core\Url, so we could just use the Link class name here too?

   On the other side, later on we use "link generator" again, then lowercase though.

4. +++ b/core/modules/system/tests/src/Functional/Common/UrlTest.php
   @@ -166,12 +167,12 @@ public function testLinkRenderArrayText() {
          $renderable_text = ['#markup' => 'foo'];
   -      $l_renderable_text = \Drupal::l($renderable_text, Url::fromUri('https://www.drupal.org'));
   +      $l_renderable_text = \Drupal::service('link_generator')->generate($renderable_text, Url::fromUri('https://www.drupal.org'));
   

   is there a specific reason for using the service here and not everywhere else? Above we also say link generator in the comment and use Link::..

5. +++ b/core/modules/views/tests/src/Kernel/Handler/FieldUrlTest.php
   @@ -61,7 +62,7 @@ public function testFieldUrl() {
    
   -    $this->assertEqual(\Drupal::l('John', Url::fromUri('base:John'))->getGeneratedLink(), $view->field['name']->advancedRender($view->result[0]));
   +    $this->assertEqual(Link::fromTextAndUrl('John', Url::fromUri('base:John'))->toString(), $view->field['name']->advancedRender($view->result[0]));
   

   this used getGeneratedLink() before, I guess that means it works due to \Drupal\KernelTests\KernelTestBase::assertEquals(), both of these changes were committed within a week, so I suspect that was done before the string cast was added.

   I guess that is there to stay and not deprecated, so seems fine.

1) I did go through a few iterations, no objections to going back to the typecase. fixed.

2) In the documentation, ->toRenderable() is recommended over ->toString() when possible, so I did switch over to it in the cases where the link was directly used in a renderable array.

3) I lowercased link generator. The reason I say it tests the link generator and not Link is because as far as I can tell the filtering is done in the Link->toString() method by way of the link generator service, that's what the assertion message says, and what it looks like from the code.

4) Again, I say link generator in the comment because the processing that is being tested is done in the link generator ->generate() through the Link->toString() method. It said "Test the link generator" before, despite accessing it through the Drupal::l method, and it's still testing the generator, even if it's accessing it through Link::toString() now.

I use the link_generator service directly when the text is a renderable array. The documentation for Link:: methods require $text to be a string. (I do use Link::fromTextAndUrl with TranslateableMarkup and FormatableMarkup objects despite the documentation, but at least those are castable to string. I couldn't bring myself to pass in an actual array.

5) yeah, weak typing on the assertion means the getGeneratedLink() isn't needed. I didn't see a reason to add it if there was weak typing in the assertion.

4. Yeah, fair enough, I would say that the current description on Link::fromTextAndUrl() is a documentation bug, there's no reason that it should support fewer things than LinkGenerator::generate(). I could almost see you working on that test and pondering just far you think you can go against the documentation ;) Should we create a follow-up for that?

5. It's not just weak typing, though, if you comment out our own explicit string type cast in KernelTestBase, it actually fails like this:

Drupal\Core\GeneratedLink Object &0000000022dcc753000000005931fa64 (
    'generatedLink' => '<a href="/John">John</a>'
    'cacheContexts' => Array &0 ()
    'cacheTags' => Array &1 ()
    'cacheMaxAge' => -1
    'attachments' => Array &2 ()
) is not instance of expected class "Drupal\views\Render\ViewsRenderPipelineMarkup".

So assertEquals() behaves somewhat unexpected here, as long as one of the arguments is a string, it is compared as a string if the other can be cast to a string. But if both are objects, they are compared as objects and they are different objects that can both be cast to the same string. Which actually makes sense, why should it bother to cast to string of it receives two objects. But that's fine, I didn't expect a change here, just wanted to write down my findings on why we used to do that and why it is no longer necessary.

I'm not 100% sure about all the render array changes. On one hand the change is clearly inline with the intentions of how toRenderable() is to be used and is correct. On the other it's kinda unexpected. That said render arrays are considered internal API as per https://www.drupal.org/core/d8-bc-policy - so I'm tentatively +1. I think a release manager should make this call.

> I'm not 100% sure about all the render array changes.

"all the" sounds like more than it is I think, if I saw that correctly, there are 3 non-test replacements, field_help() which I can't imagine anyone would try to alter, then the field storage list builder, that.. maybe? and the locale status in the language list, also an unlikely case for someone to customize?

But yeah, if a release manager feels unsure about that, it's a small thing to change it back.

The render array changes are fine with me for a minor release, so untagging. Didn't yet review the whole patch so leaving RTBC.

Committed ae248f2 and pushed to 8.8.x. Thanks!