Remove html => TRUE option from l() and link generator.

Just a reminder that only core maintainers (or someone commenting on their behalf) should add the beta blocker tag. :) In this case, I'd agree that this change, if we need to make it, is absolutely impactful enough to be a beta blocker. Let's get catch's feedback on this issue.

I think Joel added the beta blocker because this is a split off from a proper beta blocker only separate to make the parent easier to review.

There are four Drupal 8 core maintainers: Dries, webchick, alexpott, and catch. You and me and chx are core component maintainers, but it's a different responsibility. ;) The point though is that the scope of the beta release is defined by the four core maintainers. (Apologies as this is completely off-topic; just documenting it here in case anyone else reads and is confused. More details at http://buytaert.net/the-next-step-for-drupal-8-is-a-beta.)

I agree that it's better to do this as a separate issue, absolutely.

If you do this, what will be the proper way to created an image link?

#13 looks like really nice DX. So what would happen if I did l('<img src="foo">', $path) without the safe markup? Would it be autoescaped?

Clarified that it's a bit more than just it appears, you have to write the use statement as well, instead of just 'html' => TRUE...

Looking at the non-test uses first:

[tesla:drupal | Sun 15:32:18] $ grep -rnl "'html' => TRUE" * | grep -v "/Tests/"
core/includes/form.inc
core/includes/tablesort.inc
core/modules/aggregator/aggregator.theme.inc
core/modules/comment/comment.api.php
core/modules/comment/comment.module
core/modules/comment/lib/Drupal/comment/CommentViewBuilder.php
core/modules/comment/tests/modules/comment_test/comment_test.module
core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.php
core/modules/entity/lib/Drupal/entity/EntityDisplayModeListBuilder.php
core/modules/forum/forum.module
core/modules/node/lib/Drupal/node/NodeViewBuilder.php
core/modules/node/node.api.php
core/modules/shortcut/shortcut.module
core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
core/modules/user/user.module
core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php
core/modules/views/lib/Drupal/views/Plugin/views/field/Url.php
core/modules/views/views.theme.inc
core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/Rearrange.php
core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/ReorderDisplays.php
core/modules/views_ui/lib/Drupal/views_ui/ViewEditForm.php
core/modules/views_ui/views_ui.theme.inc

Here's what's using it:

• comment.module seems to use it wantonly for comment links with translated plain strings and it's not obvious to me why. Maybe I'm missing something.
• form_pre_render_actions_dropbutton(), because dropbuttons
• tablesort_header(), because tablesort header
• template_preprocess_aggregator_feed_source(), to allow for a feed image
• DbLogController::overview(), to allow sanitized markup in a truncated log message:
  

            $log_text = Unicode::truncate(Xss::filter($message, array()), 56, TRUE, TRUE);
              $message = $this->l($log_text, 'dblog.event',  array('event_id' => $dblog->wid), array('html' => TRUE));
    

  (This one will go away when the Views conversion lands.)

• EntityDisplayModeListBuilder::render(), to allow <em> tags from the t() % placeholder type.
• forum_menu_local_tasks() confuses me a little:
  

         $links['login'] = array(
            '#theme' => 'menu_local_action',
            '#link' => array(
              'title' => t('<a href="@login">Log in</a> to post new content in the forum.', array(
                '@login' => url('user/login', array('query' => drupal_get_destination())),
              )),
              'localized_options' => array('html' => TRUE),
            ),
          );
  

• NodeViewBuilder::buildLinks(), for an accessibility <span>
• hook_node_links_alter(), example code.
• shortcut_preprocess_page(), for spans
• PathBasedBreadcrumbBuilder::build():
  

            // @todo Replace with a #type => link render element so that the alter                                          
            // hook can work with the actual data.                                                                          
            $links[] = $this->l($title, $route_request->attributes->get(RouteObjectInterface::ROUTE_NAME), $route_request->attributes->get('_raw_variables')->all(), array('html' => TRUE));
  

• template_preprocess_username(), because "We do not want the l() function to check_plain() a second time."
• Views' DisplayPluginBase::optionLink(), for a span. (Note: unless I'm confused, I think this method has a bug that will put a span in a title attribute? But out of scope.)
• Views' FieldPluginBase::renderAsLink(), to perform the "render as link" functionality for fields, amidst lots of zany markup sanitization.
• Url::render(), same purpose, with somewhat less zany sanitization.
• template_preprocess_views_view_table(), for table sorting headers.
• Views' Ajax\Rearrange::buildForm(), for an Ajax-enabling <span>.
• Views' Ajax\ReorderDisplays::buildForm(), ditto
• ViewEditForm::getFormBucket() (name made me smile) for a couple accessibility <span>s and a couple other cases that aren't obvious to me.
• theme_views_ui_rearrange_filter_form() and theme_views_ui_build_group_filter_form(), also for spans.

Tagging because pretty much all the weird stuff is in Views.

This actually needs to be postponed on #1825952: Turn on twig autoescape by default, right?

And we'd have a patch (or patches) with a lot like the attached?

Ah! I get it now. Thanks @joelpittet. So e.g. looking at form_pre_render_actions_dropbutton(). It is returning a render array, but also renders the button to a string early (with an @todo):

      // Add this button to the corresponding dropbutton.
      // @todo Change #type 'dropbutton' to be based on theme_item_list()
      //   instead of links.html.twig to avoid this preemptive rendering.
      $button = drupal_render($element[$key]);
      $dropbuttons[$dropbutton]['#links'][$key] = array(
        'title' => $button,
        'html' => TRUE,
      );

So would that one would need the SafeMarkup to avoid that button string being re-escaped later, if the early rendering isn't fixed first? (Also: the @todo seems out of date as it references theme_item_list().)

Tagging for security feedback for the proposal (though it might depend on the final implementation in #1825952: Turn on twig autoescape by default and/or the actual patch).

For real.

I have reviewed it :) very seriously: who do you want to review it in light of all that transpired these last couple months?

Prefixing title to indicate how many issues this is postponed on, for beta blocker management.

I think this is OK as a release blocker.

Not convinced it's a beta blocker - the worst that happens if this gets in post-beta and your module uses html => TRUE is you have to update to avoid double-escaping no? And html => TRUE will be redundant. That seems OK to change past beta to me.

Thanks @catch!

I strongly suggest we do not change checkPlain() like that. With the current implementation in #1825952: Turn on twig autoescape by default, this would mean that once someone marks a string as safe with SafeMarkup::set(), checkPlain() can never escape it. That's a really bad idea and would make the concerns I've raised from #219 on in that issue even worse.

Note #2314599: Use title/url instead of l() for building breadcrumb partly solved the html => TRUE bit.

This seems like a nice cleanup, going to make a start on implementing this.

The approach suggested in the summary is outdated. The approach that finally was taken in #1825952: Turn on twig autoescape by default allows to manually mark strings as safe using SafeMarkup::set() so it does not require a SafeMarkup object being passed around. That makes this issue a bit simpler, and there are no changes needed in checkPlain().

My understanding is that SafeMarkup::set() is something we want to avoid if at all possible.

https://www.drupal.org/node/2311123 documents better alternatives.

Entirely possible I am not fully understanding this issue, but SafeMarkup::set() triggers my "I have heard that is problematic" sense.

SafeMarkup::set internally is not too bad and yeah the IS is outdated; you can check whether the incoming string is marked as safe easily with SafeMarkup::isSafe.

Here's an initial pass on this.

Regarding SafeMarkup::set(), using this feels very natural so I'm afraid this will proliferate in contrib as well. What would be better?

Picking this back up, I'll start by updating the summary, then will see to the failures and finally work on the ideas that @chx came up with on IRC last night.

While working on this I just found that inline templates are not actually escaping the variables. Reported it in #2330503: [sechole] Inline templates pass through unsafe strings. This issue is blocked on that, I was writing a test using an inline template and this causes it to fail.

Woops I seem to have accidentally deleted the patch. Here it is again. This is the same patch that was in #3. edit: wrong browser tab, this was intended for #2330503: [sechole] Inline templates pass through unsafe strings.

Did some more work on this.

• Introduced support for using render arrays for titles in theme_links() + added test coverage.
• Replaced all instances of SafeMarkup::set() with inline templates, except for a single case where I think SafeMarkup::set() is still warranted.
• Updated documentation for the various link generating functions and methods to let developers know that they can output HTML when using render arrays.

Didn't address the test failures yet since this is now failing as well due to #2330503: [sechole] Inline templates pass through unsafe strings.

Un-postponing because #2330503: [sechole] Inline templates pass through unsafe strings got in. Thanks a ton @pfrenssen!

You're welcome :)

Changing status to let the bot have a go at this so we have a list of failures to focus on.

Re-rolling this as part of core mentoring.

Reroll; resolved conflicts manually. Please double check the change to theme_views_ui_rearrange_filter_form(), #46 had a change to a line that looks like is now gone.

PHP Fatal error: Call to undefined function Drupal\Core\Utility\drupal_render() in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Utility/LinkGenerator.php on line 75
PHP Warning: Invalid argument supplied for foreach() in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 588

Warning: Invalid argument supplied for foreach() in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 588

Probably caused by the LinkGeneratorTest

Yeah this call to drupal_render() should be wrapped in a drupalRender() method and mocked in the test.

Unassigning myself, won't have time to work on this in the coming 2 weeks.

I can tackle this.

OK, changes since the reroll.

• Added wrapper around drupal_render().
• Added mock for the wrapper so the test wouldn't barf.
• Allowed HTML in link text to pass through unescaped if the input was a render array.

Advice needed on the proper way to handle the mocked method in the setup. I couldn't quite explain myself over IRC to get help to do this properly.

Did fetch instead of a pull when I made #56...

1. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -130,8 +129,13 @@ public function generateFromUrl($text, Url $url) {
   +    // If the input was an array, we pass the rendered output through; otherwise sanitize it.
   +    if (is_array($text)) {
   +      $text = SafeMarkup::set($variables['text']);
   +    }
   +    else {
   +      $text = SafeMarkup::escape($variables['text']);
   +    }
   

   THe comment should be wrapped to two lines. Also do we have code coverage for this change?

2. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -145,4 +149,14 @@ public function generate($text, $route_name, array $parameters = array(), array
   +    drupal_process_attached($elements);
   

   This line is being added. Do we have code coverage for it? Or a comment for why it is needed now and wasn't before?

1. Was added by me because \Drupal\Tests\Core\Utility\LinkGeneratorTest::testGenerateWithHtml() was failing after the change (HTML in the title was being escaped instead of passing through). Wrong thing to do here?

2. That is my bad. Was looking at the AJAX controller as an example. I'll remove it, and also add a @todo to reference #2182149: Move drupalRender() and drupalRenderCollectCacheTags() out of HtmlControllerBase

Will look into the other test fails.

OK, I am at a loss why these are failing.

Drupal\block\Tests\BlockTest::testThemeName() has a fail. I think the problem has to do with template_preprocess_menu_local_task(), but I don't see what is going wrong here. Mirroring what was done in seven_preprocess_menu_local_tasks () caused weird fails; I will investigate this again. The second fail was fixed by changing a ! to a @ placeholder in the string (I'll post the patch with this, and @tim.plunkett 's requests later).

Drupal\system\Tests\Entity\EntityOperationsTest::testEntityOperationAlter() has a fail that shows up twice because of the look. I don't see what is going wrong here.

Drupal\views\Tests\Plugin\DisplayTest::testDisplayPlugin() has a fail on `$this->assertRaw($this->randomString);`. No clue about this.

Advice needed on how to proceed.

1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -1023,7 +1022,11 @@ public function overrideOption($option, $value) {
   +      $text = [
   +        '#type' => 'inline_template',
   +        '#template' => '<span>{{ text }}</span>',
   +        '#context' => $text,
   +      ];
   ...
        if (!trim($text)) {
   

   In the trim() call, $text is assumed to be a string.

   And it is in HEAD. But you're turning it to an array here.

   You can probably move the trim call to the top of the function.

2. +++ b/core/themes/seven/seven.theme
   @@ -65,6 +66,34 @@ function seven_preprocess_menu_local_tasks(&$variables) {
   +    $active = array(
   +      '#type' => 'inline_template',
   +      '#template' => '<span class="visually-hidden">{{ label }}</span>',
   +      '#context' => array('label' => t('(active tab)')),
   +    );
   ...
   +    $link_text = t('!local-task-title!active', array('!local-task-title' => $title, '!active' => $active));
   

   Same thing here. You're passing $active to t(), not to l(), so it cannot be an array. Not sure why this change is needed at all, actually.

OK, make progress on this. Block tests pass. Have not started on the other fails.

I removed the changes to seven_preprocess_menu_local_tasks(), and folded them into template_preprocess_menu_local_tasks. One thing that may be objectionable is that the translation of the active tab is now split into two t() calls (which is arguably better).

Comments from #60 should be addressed, too.

Will work on remaining fails.

I started to look at `Drupal\system\Tests\Entity\EntityOperationsTest::testEntityOperationAlter()`, and I think there is a problem with the test. Since the role name is random, you need to run it a few times to get it to one with an escapable character. In HEAD and alpha-14, the HTML test page will contain double escaped text, and the test passes. See alpha14-output.png and alpha14-src.png, which show the double output. When this patch is applied, everything is escaped properly. See remove_html_true-2273923-output.png and remove_html_true-2273923-src.png.

Can someone confirm this before I change the test to

--- a/core/modules/system/src/Tests/Entity/EntityOperationsTest.php
+++ b/core/modules/system/src/Tests/Entity/EntityOperationsTest.php
@@ -44,7 +44,7 @@ public function testEntityOperationAlter() {
     $roles = user_roles();
     foreach ($roles as $role) {
       $this->assertLinkByHref($role->url() . '/test_operation');
-      $this->assertLink(format_string('Test Operation: @label', array('@label' => $role->label())));
+      $this->assertLink(format_string('Test Operation: !label', array('!label' => $role->label())));
     }
   }

Fixed inline template call / order in Drupal\views\Tests\Plugin\DisplayTest::testDisplayPlugin(). Did not include proposed change to test mentioned in #66.

Regarding `Drupal\system\Tests\Entity\EntityOperationsTest::testEntityOperationAlter()`, after a long conversation with @chx on IRC, it looks assertLink in HEAD is bogus.

In HEAD, l() is escaping the entire title, and the role name is ending up being escaped twice (see screenshots from the test output). ->assertLink does an xpath, which is returning escaped text. Therefore, the two cancel each other out, and the end result is a false pass. This example demonstrates what is going on:

function test($x) {
  $html_dom = new \DOMDocument();
  $html_dom->loadHTML('<?xml encoding="UTF-8">' . $x);
  $elements = simplexml_import_dom($html_dom);
  $r = $elements->xpath('//a[normalize-space(text())="&"]');
  var_dump($r);
}

test('<a href="foobar">&amp;</a>');

The interdiff shows the solution. The $label gets decoded, and then the xpath will reencode it, so all is well.

But let me add that entity_test_entity_operation_alter is also bogus because it misses an 'options' => ['html' => true] in the link definition. That's why the test actually passes. I will be surprised a little if #69 doesn't reveal more ... interesting ... tests but we will see.

Fixed Drupal\views\Tests\Plugin::testDisplayPlugin()

• Added String::checkPlain to an assertRaw() to match up with escaped text
• Fixed a shadowed method

ON top of that someone should take into account http://paste.ubuntu.com/8338214/ as otherwise we will have double escaping, wont' we?

1. +++ b/core/includes/common.inc
   @@ -745,6 +745,8 @@ function drupal_http_header_attributes(array $attributes = array()) {
   + *   Strings will be sanitized automatically. If you need to output HTML in the
   + *   link text you should use a render array.
   

   should this bit talk about inline_template?

2. +++ b/core/includes/menu.inc
   @@ -387,26 +387,30 @@ function template_preprocess_menu_local_task(&$variables) {
   -  $link_text = $link['title'];
   ...
   -    $link_text = t('!local-task-title!active', array('!local-task-title' => $link['title'], '!active' => $active));
   ...
   +        'title' => t($link['title']),
   

   Note: link['title'] is coming from MenuLocalTaskDefault::getTitle() which is already translated. The point of the t() used to be able to override the way how the title and the active class is connected. This t() call will introduce a lot of additional locale entries

3. +++ b/core/includes/menu.inc
   @@ -387,26 +387,30 @@ function template_preprocess_menu_local_task(&$variables) {
   +        'active' => t('(active tab)'),
   

   Did you considered to use t in the template directly?

4. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -19,7 +19,6 @@
   -use Symfony\Component\DependencyInjection\Exception\RuntimeException as DependencyInjectionRuntimeException;
   

   This change seems to be a little bit out of scope, I don't care but just avoid those, if possible.

.

Everything from @dawehner's comments in #72 should be addressed.

Reroll of #75 so it applies cleanly to HEAD.

@pwolanin, do you need a re-roll of this? Was going to wait for all of the commits from DrupalConAms to get in before I did.

Talked about this in core-mentoting, will reroll. Hoping to have done in today or tomorrow.

Reroll against dbea832. Throwing at the testbot to see what barfs before running specific tests locally.

Fixed Drupal\block\Tests\BlockTest and Drupal\Tests\Core\Utility\LinkGeneratorTest. Want to see what TestBot says about the other two problems.

Will followup pending on what happens, because the change to block.module may need discussion and I need time to summarize it. The change to Drupal\Tests\Core\Utility\LinkGeneratorTest was related to the reroll for the Url changes.

OK, all good. Other two problems must have been from the earlier oopsie to core.

The patch includes this:

diff --git a/core/modules/block/block.module b/core/modules/block/block.module
index cbf56ab..0b760af 100644
--- a/core/modules/block/block.module
+++ b/core/modules/block/block.module
@@ -42,7 +42,7 @@ function block_help($route_name, RouteMatchInterface $route_match) {
     $demo_theme = $route_match->getParameter('theme') ?: \Drupal::config('system.theme')->get('default');
     $themes = list_themes();
     $output = '<p>' . t('This page provides a drag-and-drop interface for adding a block to a region, and for controlling the order of blocks within regions. To add a block to a region, or to configure its specific title and visibility settings, click the block title under <em>Place blocks</em>. Since not all themes implement the same regions, or display regions in the same way, blocks are positioned on a per-theme basis. Remember that your changes will not be saved until you click the <em>Save blocks</em> button at the bottom of the page.') . '</p>';
-    $output .= '<p>' . \Drupal::l(t('Demonstrate block regions (!theme)', array('!theme' => $themes[$demo_theme]->info['name'])), new Url('block.admin_demo', array('theme' => $demo_theme))) . '</p>';
+    $output .= '<p>' . \Drupal::l(t('Demonstrate block regions (@theme)', array('@theme' => $themes[$demo_theme]->info['name'])), new Url('block.admin_demo', array('theme' => $demo_theme))) . '</p>';
     return $output;
   }
 }

I have no idea what is going on here, or what the proper thing to do is. Before the Url change, that function used l() with ! placeholders w/o problem. Using \Drupal::l() with ! placeholders makes Fixed Drupal\block\Tests\BlockTest fail, and also makes the theme name an XSS liability. Part of the patch is to check escaped text in a few places with

$this->assertRaw(String::checkPlain($foo));

to ensure that it is actually escaped (shameless plug for #2340785: Create proper test method for determining if text has been escaped properly). So, changing the output in this case to use escaped placeholders rather than the fudge the test seemed the proper thing to do here, but I am open to suggestions.

Can you post an interdiff? That's very helpful to see what has changed ;)

1. +++ b/core/includes/common.inc
   @@ -697,6 +697,26 @@ function drupal_http_header_attributes(array $attributes = array()) {
   + * Links that require HTML in the link text (such as an image) need to use an
   + * a render array.  For example:
   + * @code
   

   "use an a render array" has too much "an". There are also two spaces following the period.

2. +++ b/core/includes/common.inc
   @@ -697,6 +697,26 @@ function drupal_http_header_attributes(array $attributes = array()) {
   + * l($image, $path);
   + * @endcode
   

   l() has been renamed to _l().

3. +++ b/core/includes/common.inc
   @@ -697,6 +697,26 @@ function drupal_http_header_attributes(array $attributes = array()) {
   + * $text = array(
   + *   #type' => 'inline_template',
   + *   '#template' => '<strong>{{ foo }}</strong>',
   + *   '#context' => array('foo' => t('Foo')),
   + * );
   

   This has a syntax error: the #type declaration misses its opening quote.

4. +++ b/core/includes/common.inc
   @@ -697,6 +697,26 @@ function drupal_http_header_attributes(array $attributes = array()) {
   + * l($text, $path);
   + * @endcode
   

   l() has been renamed to _l().

5. +++ b/core/includes/tablesort.inc
   @@ -50,24 +52,21 @@ function tablesort_header(&$cell_content, array &$cell_attributes, array $header
   +    $cell_content = _l($text, current_path(), array(
          'attributes' => array('title' => $title),
          'query' => array_merge($ts['query'], array(
            'sort' => $ts['sort'],
            'order' => $cell_content,
          )),
   -      'html' => TRUE,
        ));
   

   Maybe now that these lines are touched, replace the deprecated call to _l()?

6. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -135,9 +133,25 @@ public function generate($text, Url $url) {
   -    // Sanitize the link text if necessary.
   -    $text = $variables['options']['html'] ? $variables['text'] : String::checkPlain($variables['text']);
   +    // If the input is an array, pass the rendered output through; otherwise
   +    // sanitize it.
   +    if (is_array($text)) {
   +      $text = SafeMarkup::set($variables['text']);
   +    }
   +    else {
   +      $text = SafeMarkup::escape($variables['text']);
   +    }
   

   This is probably not correct. The text in $variables['text'] could have been altered and be different from the original version that is stored in $text.

   Also, is it really the best solution to use SafeMarkup::set() here? Using that method is discouraged. If this is a good solution, then add some documentation explaining why it is acceptable to use SafeMarkup::set() here.

7. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -135,9 +133,25 @@ public function generate($text, Url $url) {
   +  protected function drupalRender(&$elements, $is_recursive_call = FALSE) {
   +    $output = drupal_render($elements, $is_recursive_call);
   +    return $output;
   +  }
   

   You can just return drupal_render(...);.

8. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -85,7 +85,7 @@ function _testImageFieldFormatters($scheme) {
   -    $default_output = '<a href="' . file_create_url($image_uri) . '">' . drupal_render($image) . '</a>';
   +    $default_output = _l($image, file_create_url($image_uri));
        $this->drupalGet('node/' . $nid);
   

   Now that this code is touched, maybe replace the deprecated call _l() with LinkGenerator::generate().

9. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
   @@ -115,7 +115,7 @@ public function _testResponsiveImageFieldFormatters($scheme) {
   -    $default_output = '<a href="' . file_create_url($image_uri) . '">' . drupal_render($image) . '</a>';
   +    $default_output = _l($image, file_create_url($image_uri));
        $this->drupalGet('node/' . $nid);
   

   Now that this is touched, maybe replace _l().

10. +++ b/core/modules/simpletest/src/AssertContentTrait.php
    @@ -284,6 +284,9 @@ protected function getAllOptions(\SimpleXMLElement $element) {
    +    // The ->xpath will escape entities, so we need to decode them first to
    +    // avoid double escaping leading to failed tests.
    +    $label = html_entity_decode($label);
    

    It is not common to refer to methods in the current scope as ->xpath. More common would be ::xpath() or simply xpath() or $this->xpath().

11. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -1030,19 +1030,23 @@ public function overrideOption($option, $value) {
    +      $text = t('Broken field');
         }
    

    Use $this->t() instead of t().

12. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -1125,13 +1129,13 @@ public function optionsSummary(&$categories, &$options) {
    -        'title' => $this->t('Machine Name'),
    -        'value' => !empty($this->display['new_id']) ? String::checkPlain($this->display['new_id']) : String::checkPlain($this->display['id']),
    -        'desc' => $this->t('Change the machine name of this display.'),
    +        'title' => t('Machine Name'),
    +        'value' => !empty($this->display['new_id']) ? $this->display['new_id'] : $this->display['id'],
    +        'desc' => t('Change the machine name of this display.'),
           );
    

    Let's stick to $this->t() instead of using the venerable t().

13. +++ b/core/modules/views/src/Plugin/views/field/Url.php
    @@ -45,7 +45,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    -      return _l($this->sanitizeValue($value), $value, array('html' => TRUE));
    +      return _l($this->sanitizeValue($value), $value);
         }
    

    Maybe replace _l() now that this is touched.

14. +++ b/core/modules/views_ui/src/Form/Ajax/Rearrange.php
    @@ -124,7 +124,19 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -        '#suffix' => _l('<span>' . $this->t('Remove') . '</span>', 'javascript:void()', array('attributes' => array('id' => 'views-remove-link-' . $id, 'class' => array('views-hidden', 'views-button-remove', 'views-remove-link'), 'alt' => $this->t('Remove this item'), 'title' => $this->t('Remove this item')), 'html' => TRUE)),
    +        '#suffix' => _l(
    +          array(
    +            '#type' => 'inline_template',
    

    Maybe replace _l().

15. +++ b/core/modules/views_ui/views_ui.theme.inc
    @@ -166,7 +165,19 @@ function theme_views_ui_build_group_filter_form($variables) {
    -      'remove' => drupal_render($form['group_items'][$group_id]['remove']) . _l('<span>' . t('Remove') . '</span>', 'javascript:void()', array('attributes' => array('id' => 'views-remove-link-' . $group_id, 'class' => array('views-hidden', 'views-button-remove', 'views-groups-remove-link', 'views-remove-link'), 'alt' => t('Remove this item'), 'title' => t('Remove this item')), 'html' => true)),
    +      'remove' => drupal_render($form['group_items'][$group_id]['remove']) . _l(
    +        array(
    +          '#type' => 'inline_template',
    

    Maybe replace _l().

Thanks. Most of these are easy, assuming the conflicts from further rebasing are minimized when I make the patch. I should have something ready tonight (EDT).

@pfrenssen, regarding item 6 in #86, that was added to address the test for LinkGenerator. IIRC, the issue was the test with the inline template was failing with double escaped output (or barfing outright). Since we are explicitly rendering the text near the top of LinkGenerator::generate() now to account for inline Twig templates, and therefore should be a safe string already per the final lines of drupal_render(), perhaps the safest thing to do is:

// If we don't have a safe string, sanitize it.
if (SafeMarkup::isSafe($variables['text'])) {
  $text = $variables['text'];
}
else {
  $text = SafeMarkup::escape($variables['text']);
}

which take into account the text being altered to an unsafe string.

Yeah most are just minor cleanups, this is starting to look pretty good.

Your proposal to check if the string is safe before escaping looks OK too!

OK, throwing this as the TestBot. Tests work locally for me.

Done from #86 above:

1,2,3,4,7,10,11,12 were all easy changes.

5 looks like it was part of HEAD already when I did a pull+rebase.

8,9 are done and use the injected link generator.

13 uses \Drupal::l()

Not done from #86 above:

My idea for 6 didn't work. I need to spend some time with the debugger to figure out what is going wrong here. My guess is that the string is being set as unsafe again as a result of the alter. That will be a tricky situation if it is the case.

14 and 15 use javascript:void as the URL, which I don't think can be handled yet with LinkGenerator?

Addressed item 6 from #86 above. Looks like the root problem if my idea was insufficient mocking of what drupal_render() would do. Tests work locally.

+++ b/core/includes/tablesort.inc
@@ -51,24 +56,17 @@ function tablesort_header(&$cell_content, array &$cell_attributes, array $header
-      $tablesort_indicator = array(
+      $text['image'] = array(
         '#theme' => 'tablesort_indicator',
         '#style' => $ts['sort'],
       );
-      $image = drupal_render($tablesort_indicator);
     }
     else {
       // If the user clicks a different header, we want to sort ascending initially.
       $ts['sort'] = 'asc';
-      $image = '';
     }
-    $cell_content = \Drupal::l($cell_content . $image, new Url('<current>', [], [
-      'attributes' => array('title' => $title),
-      'query' => array_merge($ts['query'], array(
-        'sort' => $ts['sort'],
-        'order' => $cell_content,
-      )),
-      'html' => TRUE,
+    $cell_content = \Drupal::l($text, new Url('<current>', [
+      'attributes' => [ 'title' => $title ],

This looks like the image will not change direction when the table sort order is flipped. $ts['sort'] is changed after the image has been defined.
Removing the query arguments doesn't seem right either.
Seeing that the tests pass with these changes in place it seems we are missing test coverage on the tablesort functionality.

Restored the tablesort functionality.

Yeah, that does look better now. I suspect this was the result of a bad conflict merge on my part.

Great work!

1. +++ b/core/includes/common.inc
   @@ -697,6 +697,26 @@ function drupal_http_header_attributes(array $attributes = array()) {
   + * _l($image, $path);
   ...
   + * _l($text, $path);
   

   Calling _l() is is certainly not the recommended way ...

2. +++ b/core/includes/menu.inc
   +++ b/core/includes/menu.inc
   @@ -324,26 +324,27 @@ function template_preprocess_menu_local_task(&$variables) {
   
   @@ -324,26 +324,27 @@ function template_preprocess_menu_local_task(&$variables) {
   +    // Build up an inline template which will be autoescaped.
   +    $link_text = array(
   +      '#type' => 'inline_template',
   +      '#template' => '{{ title }}<span class="visually-hidden">{% trans %}(active tab){% endtrans %}></span>',
   +      '#context' => array('title' => $link['title']),
   +    );
   

   Maybe I am dump but is there a reason we cannot at least move the text itself into the main twig file? This would allow much better speed as well as better adaptability.

   On the other hand this is maybe out of scope.

3. +++ b/core/includes/tablesort.inc
   @@ -51,25 +56,28 @@ function tablesort_header(&$cell_content, array &$cell_attributes, array $header
   -    $cell_content = \Drupal::l($cell_content . $image, new Url('<current>', [], [
   -      'attributes' => array('title' => $title),
   -      'query' => array_merge($ts['query'], array(
   -        'sort' => $ts['sort'],
   -        'order' => $cell_content,
   -      )),
   -      'html' => TRUE,
   -    ]));
   ...
   +        [ 'attributes' => [ 'title' => $title ] ],
   +        [ 'query' => array_merge(
   +          $ts['query'], [ 'sort' => $ts['sort'], 'order' => $cell_content ]
   +        )]
   

   If you compare the two approaches we now pass along the title arguments as parameters, this can't work.

4. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -135,9 +133,23 @@ public function generate($text, Url $url) {
   +    // If we don't have a safe string, sanitize it.
   +    if (!SafeMarkup::isSafe($variables['text'])) {
   +      $text = SafeMarkup::escape($variables['text']);
   +    }
   +    else {
   +      $text = $variables['text'];
   +    }
   

   Isn't that exactly the same as $text = SafeMarkup::escape($variables['text'])?

5. +++ b/core/lib/Drupal/Core/Utility/LinkGeneratorInterface.php
   @@ -28,29 +28,11 @@
       *   The URL object used for the link. Amongst its options, the following may
       *   be set to affect the generated link:
   -   *   - attributes: An associative array of HTML attributes to apply to the
   -   *     anchor tag. If element 'class' is included, it must be an array; 'title'
   -   *     must be a string; other elements are more flexible, as they just need
   -   *     to work as an argument for the constructor of the class
   -   *     Drupal\Core\Template\Attribute($options['attributes']).
   -   *   - html: Whether $text is HTML or just plain-text. For
   -   *     example, to make an image tag into a link, this must be set to TRUE, or
   -   *     you will see the escaped HTML image tag. $text is not sanitized if
   -   *     'html' is TRUE. The calling function must ensure that $text is already
   -   *     safe. Defaults to FALSE.
   -   *   - language: An optional language object. If the path being linked to is
   -   *     internal to the site, $options['language'] is used to determine whether
   -   *     the link is "active", or pointing to the current page (the language as
   -   *     well as the path must match).
   -   *   - 'set_active_class': Whether this method should compare the $route_name,
   -   *     $parameters, language and query options to the current URL to determine
   -   *     whether the link is "active". Defaults to FALSE. If TRUE, an "active"
   -   *     class will be applied to the link. It is important to use this
   -   *     sparingly since it is usually unnecessary and requires extra
   -   *     processing.
       *
   

   mh why is all the documentation removed here?

6. +++ b/core/modules/aggregator/aggregator.theme.inc
   @@ -50,6 +50,36 @@ function template_preprocess_aggregator_feed(&$variables) {
   +function template_preprocess_aggregator_feed_source(&$variables) {
   ...
   +  $feed_icon = array(
   +    '#theme' => 'feed_icon',
   +    '#url' => $feed->getUrl(),
   +    '#title' => t('!title feed', array('!title' => $feed->label())),
   +  );
   +  $variables['source_icon'] = drupal_render($feed_icon);
   

   Dump question, do we need to call drupal_render() here or could we just do it implicit inside the template?

7. +++ b/core/modules/aggregator/aggregator.theme.inc
   @@ -50,6 +50,36 @@ function template_preprocess_aggregator_feed(&$variables) {
   +    $image = array(
   +      '#theme' => 'image',
   +      '#path' => $feed->getImage(),
   +      '#alt' => $feed->label(),
   +    );
   +    $variables['source_image'] = l($image, $feed->getWebsiteUrl(), array('attributes' => array('class' => 'feed-image')));
   

   We also have a replacement for l() inside a template so should we use it there?

8. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -26,6 +27,13 @@ class ImageFieldDisplayTest extends ImageFieldTestBase {
   +    $this->link_generator = $this->container->get('link_generator');
   
   @@ -85,7 +93,8 @@ function _testImageFieldFormatters($scheme) {
   +    $default_output = $this->link_generator->generate($image, Url::fromUri(file_create_url($image_uri)));
   
   +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
   @@ -31,6 +32,9 @@ class ResponsiveImageFieldDisplayTest extends ImageFieldTestBase {
   +    $this->link_generator = $this->container->get('link_generator');
   
   @@ -115,7 +119,9 @@ public function _testResponsiveImageFieldFormatters($scheme) {
   +    $default_output = $this->link_generator->generate($image, Url::fromUri(file_create_url($image_uri)));
   

   nitpick: let's use camelCase here for the linkGenerator .

9. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
   @@ -115,7 +119,9 @@ public function _testResponsiveImageFieldFormatters($scheme) {
   +    //$default_output = _l($image, file_create_url($image_uri));
   

   Let's drop that line.

10. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -1126,12 +1130,12 @@ public function optionsSummary(&$categories, &$options) {
    -        'value' => !empty($this->display['new_id']) ? String::checkPlain($this->display['new_id']) : String::checkPlain($this->display['id']),
    +        'value' => !empty($this->display['new_id']) ? $this->display['new_id'] : $this->display['id'],
             'desc' => $this->t('Change the machine name of this display.'),
    ...
    -    $display_comment = String::checkPlain(drupal_substr($this->getOption('display_comment'), 0, 10));
    +    $display_comment = drupal_substr($this->getOption('display_comment'), 0, 10);
         $options['display_comment'] = array(
    
    @@ -1325,7 +1329,7 @@ public function optionsSummary(&$categories, &$options) {
    -          $link_display = String::checkPlain($displays[$display_id]['display_title']);
    +          $link_display = $displays[$display_id]['display_title'];
    
    @@ -1366,7 +1370,7 @@ public function optionsSummary(&$categories, &$options) {
     
    -    $css_class = String::checkPlain(trim($this->getOption('css_class')));
    +    $css_class = trim($this->getOption('css_class'));
    

    It would be great if you could check whether this not accidentally introduces some secholes. Just try out to insert some basic

    alert(123);

    in there.

Thanks Daniel. I'll post a patch later with the easy stuff, and then we can discuss the a few of these points (I think just 2 and 3, but maybe 6) and how we should proceed.

Edit, It take the timeframe back. There are a lot of conflicts from the rebase, which is where some of these errors keep creeping back in. Will post patch when I can.

Addressed from #94

4, 5, 6, 8, 9, 10 (via visual verification with an XSS vector)

Not addressed from #94

1. This is part of the documentation for _l(), so the usage shows how to use _l(), even though it is deprecated. The proper replacements are documented further down.

2,7. Awaiting feedback on whether we want to do this in scope for this patch. 2 would also ripple into the core themes and potentially affect any Drupal 8 themes.

3. Defer to @pfrenssen on this

The attached interdiff is after I rebased, but was hand edited to remove a few self-comments I accidentally committed while doing the merges.

I think TestBot will like this.

Ok, the fail in Drupal\block\Tests\BlockTest was from a bad conflict merge on my part. Yay tests catching it.

Pretty sure the fail in Drupal\views_ui\Tests\DisplayPathTest is from #2353347: Random failure in DisplayPathTest.

Two interdiffs attached, one against #92 and one showing the change to fix the merge oopsie.

This is a partial review of some things I noticed. Thank you for keeping this big issue rolling.

1. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -135,9 +133,18 @@ public function generate($text, Url $url) {
   +  /**
   +   * Wraps drupal_render().
   +   *
   +   * @todo: Remove as part of https://drupal.org/node/2182149
   +   */
   +  protected function drupalRender(&$elements, $is_recursive_call = FALSE) {
   +    return drupal_render($elements, $is_recursive_call);
   +  }
   

   I am not sure why this is here. The issue mentioned is "closed, works as designed" #2182149: Move drupalRender() and drupalRenderCollectCacheTags() out of HtmlControllerBase

2. +++ b/core/lib/Drupal/Core/Utility/LinkGeneratorInterface.php
   @@ -50,8 +47,7 @@
   -   *     processing.
   -   *
   +   *     processing.   *
   

   There is a comment typo here. While fixing it, remember the blank link after the last @param.

3. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -27,6 +28,20 @@ class ImageFieldDisplayTest extends ImageFieldTestBase {
   +   * The injected link generator.
   +   *
   +   * @var \Drupal\Core\Utility\LinkGenerator
   +   */
   ...
   +
   +  protected function setUp() {
   +    parent::setUp();
   +
   +    // Get the injected dependencies.
   +    $this->linkGenerator = $this->container->get('link_generator');
   +  }
   

   LinkGenerator is not injected, the container is. I would recommend changing this comment to "The link generator."

4. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
   @@ -31,6 +32,9 @@ class ResponsiveImageFieldDisplayTest extends ImageFieldTestBase {
   +    // Get the injected dependencies.
   

   This comment is unnecessary.

I'll fixe these, and I suspect I will have another batch of conflicts to merge first.

@cilefen, your first point in #99 was added in the patch in #56 for test mocking purposes.

The comment was copied from \Drupal\Core\Ajax\AjaxResponseRenderer, which had to do something similar for testing purposes.

I see. I guess the @todo should come out in that case.

No merge conflicts when I rebased.

Addressed all comments in #99. In addition, I changed the member properties mentioned in 3 and 4 to use camel case, and to be protected instead of private. Also added the explicit declaration w/ type hint to ResponsiveImageFieldDisplayTest to mirror ImageFieldDisplayTest. Tests pass locally.

1. +++ b/core/includes/common.inc
   @@ -664,6 +664,26 @@ function drupal_http_header_attributes(array $attributes = array()) {
   + * _l($image, $path);
   ...
   + * _l($text, $path);
   

   _l is still no recommended API, se earlier review :( If there is one place than it should be the LinkGeneratorInterface where this specific piece of documentation should exist.

2. +++ b/core/includes/menu.inc
   @@ -324,26 +324,27 @@ function template_preprocess_menu_local_task(&$variables) {
   -  $link_text = $link['title'];
   ...
   +    '#title' => is_array($link_text) ? drupal_render($link_text) : String::checkPlain($link_text),
   

   Did you tried out whether this change maybe results in a double escaping problem?

3. +++ b/core/includes/theme.inc
   @@ -991,7 +988,7 @@ function template_preprocess_links(&$variables) {
   +        '#title' => is_array($link['title']) ? drupal_render($link['title']) : SafeMarkup::escape($link['title']),
   

   Is there a reason we use once String::checkPlain and in the other case SafeMarkup::escape?

4. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -75,8 +75,7 @@ public function generate($text, Url $url) {
   +      'text' => is_array($text) ? $this->drupalRender($text) : $text,
   

   So yeah we wrap that for better unit test support.

5.     new Url(
   +        '<current>',
   +        [ 'attributes' => [ 'title' => $title ] ],
   

   This is still wrong ... just try it out, it will have no proper title attribute.

New patch. To address points some of the points in #102.

1. Per some IRC advice, I fixed the documentation for _l(), restoring it back to better align with HEAD, minus the 'html' option. I took a pass at expanding the example code in the @deprecated section to show the LinkGenerator. Rewrites here appreciated.

2. The explicit plaining is a result of #2338081: Local Tasks, Actions, and Contextual links mark strings from derivatives (or alter hooks) as safe and translated. Added a @todo that references this. chx and I discovered this one night. Though my name is on that bug report, he wrote the issue summary with all of the details. I don't fully grasp the scope of that.

5. Got to the root of this. This is HEAD right now in core/includes/tablesort.inc:65

$cell_content = \Drupal::l($cell_content . $image, new Url('<current>', [], [
  'attributes' => array('title' => $title),
  'query' => array_merge($ts['query'], array(
    'sort' => $ts['sort'],
    'order' => $cell_content,
  )),
  'html' => TRUE,
]));
<del>

Neither 'attributes' nor 'html' are valid in the $options parameter for the Url constructor, but there they are. This carried over into the patch. It is fixed in this patch.

@mpdonadio
Thank you for the hard work on this quite important security improvement issue!

I don't want to sound pedantic but still, you are introducing a regression compared to something which worked before. Go to "admin/content/comment" and you will see that your
patch removes the title attributes, as expected.

Before

After

@dawehner, it's not about you being pedantic (which is the point of peer review), It is me being dense :) and perhaps an opportunity to add some test coverage for tablesort in the future.

Title is back: visually verified the browser tooltip and saw the attribute in a View Source on the admin/content page.

@mpdonadio
Thank you! Do you want to open a follow up to add explicit test coverage for the title?

I made #2357997: Add test coverage for tablesort header link titles and will make the patch.

Reroll

1. +++ b/core/includes/common.inc
   @@ -691,12 +690,23 @@ function drupal_http_header_attributes(array $attributes = array()) {
   + * // Using the static methods.
     * $installer_url = \Drupal\Core\Url::fromUri('base://core/install.php')->toString();
     * $installer_link = \Drupal::l($text, $installer_url);
     * $external_url = \Drupal\Core\Url::fromUri('http://example.com', ['query' => ['foo' => 'bar']])->toString();
     * $external_link = \Drupal::l($text, $external_url);
     * $internal_url = \Drupal\Core\Url::fromRoute('system.admin')->toString();
     * $internal_link = \Drupal::l($text, $internal_url);
   + *
   + * // Using the 'link_generator' service.
   + * $link_generator = \Drupal::service('link_generator');
   + * $installer_url = \Drupal\Core\Url::fromUri('base://core/install.php');
   + * $installer_link = $link_generator->generate($text, $installer_url);
   + * $external_url = \Drupal\Core\Url::fromUri('http://example.com', ['query' => ['foo' => 'bar']]);
   + * $external_link = $link_generator->generate($text, $external_url);
   + * $internal_url = \Drupal\Core\Url::fromRoute('system.admin');
   + * $internal_link = $link_generator->generate($test, $internal_url);
   +
   

   Mh, i don't get that ... you already have covered the exact same usecases above with \Drupal::l() ?

2. +++ b/core/modules/aggregator/aggregator.theme.inc
   @@ -50,6 +50,35 @@ function template_preprocess_aggregator_feed(&$variables) {
   +    '#title' => t('!title feed', array('!title' => $feed->label())),
   

   I am not convinced by this change ... doens't t() gets marked as safe, so that this introduces a security issue here?

+++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
@@ -344,10 +352,14 @@ public function testGenerateWithHtml() {
diff --git a/core/themes/seven/seven.theme b/core/themes/seven/seven.theme

diff --git a/core/themes/seven/seven.theme b/core/themes/seven/seven.theme
index 4863525..a0818b4 100644

index 4863525..a0818b4 100644
--- a/core/themes/seven/seven.theme

--- a/core/themes/seven/seven.theme
+++ b/core/themes/seven/seven.theme

+++ b/core/themes/seven/seven.theme
+++ b/core/themes/seven/seven.theme
@@ -5,8 +5,9 @@

@@ -5,8 +5,9 @@
  * Functions to support theming in the Seven theme.
  */
 
-use Drupal\Component\Utility\Xss;
+use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\String;
+use Drupal\Component\Utility\Xss;
 use Drupal\Core\Form\FormStateInterface;
 
 /**

This change is not needed here ...

Will address comments and look into the feed label thing. Also did confirm that #2324371: Fix common HTML escaped render #key values due to Twig autoescape didn't impact this.

Patch to address comments from #111

1. Per IRC conversation, moved the link_generator examples to LinkGeneratorInterface

2. This function was removed in #2138115: Split aggregator theme functions to a separate file. It is gone from patch now.

3. Reverted to match HEAD.

So that issue could still go in with a) leaving html => TRUE/FALSE as a deprecated option, removing all usage of that API from core (setting a good example) and using escape:: instead of checkPlain. No BC break, but still good example and no double escape for already escaped text.

The BUG here is that putting in already escaped text, gets double escaped.

This is something you might still expect from l(), but from the new link generator?

That bug and empowering core to be able to remove all html => TRUE cases from the code base (to set a good example => secure by default!) is what should really be fixed here.

catch seemed to have been okay with the BC break for the html => TRUE, but I am generally okay for this issue to go in with or without BC break as long as the usage of html => TRUE is very very much discouraged and deprecated.

On the criticalness itself, consider:

At least in D7 that code would have been a security hole:

l('<span>' . $user_input . '</span>', 'path', array('html' => TRUE));

We need to decide if we really want to allow / encourage that mistake in the future given we have so much more powerful tools and do everything to prevent people from shooting themselves in the foot now. Especially with the new link generator being a new and non-finalized API still anyway (I could understand resentments more if l() would still exist like in D7, but it does not.)

Rerolled after #2364161: Replace nearly all existing _l calls got in. We will need to replace all remaining calls to _l() with \Drupal::l().

1. +++ b/core/includes/menu.inc
   @@ -329,26 +329,36 @@ function template_preprocess_menu_local_task(&$variables) {
   +  if (is_array($link_text)) {
   +    $title = drupal_render($link_text);
   +  }
   +  else {
   +    // @todo Remove expicit plaining when https://www.drupal.org/node/2338081
   +    //   gets fixed.
   +    $title = String::checkPlain($link_text);
   +  }
   +
   

   • This if-else block is redundant, in the code block above it the same if-else is already done so this code can be integrated there.
   • Typo: "expicit".
   • "plaining" is not a real word :) A better choice would be "escaping".

2. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -135,9 +133,16 @@ public function generate($text, Url $url) {
   +  /**
   +   * Wraps drupal_render().
   +   */
   +  protected function drupalRender(&$elements, $is_recursive_call = FALSE) {
   +    return drupal_render($elements, $is_recursive_call);
   

   I would still document the properties here, even though it is just a simple wrapper.

3. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -27,6 +28,19 @@ class ImageFieldDisplayTest extends ImageFieldTestBase {
   +  protected function setUp() {
   +    parent::setUp();
   +
   +    $this->linkGenerator = $this->container->get('link_generator');
   +  }
   

   Document with @inheritdoc.

4. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
   @@ -27,11 +28,20 @@ class ResponsiveImageFieldDisplayTest extends ImageFieldTestBase {
   +  /**
       * Drupal\simpletest\WebTestBase\setUp().
       */
      protected function setUp() {
        parent::setUp();
   

   Now that we're touching this, let's fix this docblock to @inheritdoc.

5. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -1024,19 +1024,23 @@ public function overrideOption($option, $value) {
   -    return \Drupal::l($text, new Url('views_ui.form_display', ['js' => 'nojs', 'view' => $this->view->storage->id(), 'display_id' => $this->display['id'], 'type' => $section], array('attributes' => array('class' => array('views-ajax-link', $class), 'title' => $title, 'id' => drupal_html_id('views-' . $this->display['id'] . '-' . $section)), 'html' => TRUE)));
   +    return \Drupal::l($text, new Url('views_ui.form_display', ['js' => 'nojs', 'view' => $this->view->storage->id(), 'display_id' => $this->display['id'], 'type' => $section], array('attributes' => array('class' => array('views-ajax-link', $class), 'title' => $title, 'id' => drupal_html_id('views-' . $this->display['id'] . '-' . $section)))));
   

   Coding standards: split this array declaration to multiple lines.

Reviewed the top 75% of the patch.

What is the status of this patch actually getting in, now that Twig autoescape is committed?

I'm happy to fix all of this, though, and the failed tests (which based on the message should just be simple fixes).

Chances are high it will get in - but possibly with a not-recommended-you-are-on-your-own BC layer.

This just address the fails / exceptions from remove_html_true-2273923-117.patch so there is hopefully something cleaner to review. The changes mainly brought some bits closer to HEAD.

I'm neutral on the bc layer. We can always defer removal of that to a follow-up if the usages are done here.

+++ b/core/modules/views_ui/views_ui.theme.inc
@@ -146,6 +146,7 @@
+    debug($form['group_items'][$group_id]['remove']);

Minor nitpick ...This needs to be removed in the next iteration.

A bunch of commits got made against HEAD and #122 no longer applies. I will reroll and address comments in #119 and #124, and any other reviews that come in before I am ready to post something new.

This addresses the reroll, and #119 and #124. I should have done the reroll first, but noticed after I had done some things, and can't get a clean interdiff.

I found a bunch more places with `'html' => TRUE` that I will purge...

Fixed failing tests from above; found a few more cases of `'html' => TRUE` that I removed, and tests pass locally for me.

I am going to put some debug exceptions in UrlGenerator to see if any other calls have been missed, and post that to the IGNORE issue.

Setting back to needs review based on the re-test passing. I think this was a spurious fail. I ran \Drupal\system\Tests\Entity\EntityOperationsTest about 30 times locally and they all passed.

@mpdonadio, thanks for your awesome work on this issue!

No problem. I will post the next patch on Monday or Tuesday. Via check+test exception in LinkGenerator, I found one more case of setting HTML (It was a FASLSE, though, in a link formatter), but I want to wait for everything from BADCamp to get in to see if there are more merge conflicts.

Yes, indeed thanks so much. I have this patch open for later review, but waiting for BAD camp patches to settle down is definitely a good idea!

OK, I think this covers all current cases of HTML from being sent to the link generator.

+++ b/core/modules/views/src/Plugin/views/field/Url.php
@@ -45,7 +46,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-      return _l($this->sanitizeValue($value), $value, array('html' => TRUE));
+      return \Drupal::l($this->sanitizeValue($value), DrupalUrl::fromUri('base://' . $value));

I consider this fix to be wrong, given that it won't run path aliases anymore.

I'll look into this particular use, but code blips like this

$link = \Drupal::l($text, Url::fromUri('base://' . $path));

are used in a bunch of places in Views and Views UI in HEAD, and #2364157: Replace most existing _url calls with Url objects introduce even more.

are used in a bunch of places in Views and Views UI in HEAD, and #2364157: Replace all existing _url calls introduce even more.

Well ... these are cases where you really don't have a route name/parameter available, so the actual proper
way here would be to use URL objects in views fields everywhere possible (route names or unrouted urls).

In general I think linking to /node/1 should resolve the path alias, shouldn't it?

Rerolled patch against latest HEAD.

triggering testbot

+++ b/core/modules/views/src/Plugin/views/field/Url.php
@@ -45,7 +46,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
     if (!empty($this->options['display_as_link'])) {
-      return _l($this->sanitizeValue($value), $value, array('html' => TRUE));
+      return \Drupal::l($this->sanitizeValue($value), DrupalUrl::fromUri('base://' . $value));
     }

I'm sorry, but I don't think this is solved yet.
We rather need support for URL objects and use them in views somehow, as this will give us support for path aliases again.

Would something like this work?

public function render(ResultRow $values) {
  $value = $this->getValue($values);
  if (!empty($this->options['display_as_link'])) {
    $url = \Drupal::service('path.validator')->getUrlIfValidWithoutAccessCheck($value);
 
    if ($url === FALSE) {
      return \Drupal::l($this->sanitizeValue($value), DrupalUrl::fromUri('base://' . $value));
    }
    else {
      return \Drupal::l($this->sanitizeValue($value), $url);
    } 
  }

I was confused about #142 so I asked @dawehner for input.

This Drupal\views\Plugin\views\field\Url handler is used to optionally render a URL field as a link. A URL field is a pure URL string, it does not have any metadata like a title, attributes or anything. In core it is only used in the Aggregator module, in the Feed display of the Aggregator Sources view. In this particular use case it is only possible to enter fully qualified URLs which works fine.

@dawehner's concern is that if a field allows relative URLs (like path aliases) then these should be rendered correctly as well. It looks like @mpdonadio's suggestion is on the right path. I will have a look and see if we can get some test coverage for this.

I just thought of another use case: an administrator might enter a dynamic link to a local path that is not handled by Drupal, and this should be handled as well. So we should support:

• External URLs
• Internal non-aliases URLs: node/1, /node/1
• Internal aliased URLs
• Internal URLs not governed by Drupal
• Special cases like <front>

The suggested approach from #143 was indeed correct. Rolled it in the patch and extended test coverage.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -135,9 +133,27 @@ public function generate($text, Url $url) {
+  protected function drupalRender(&$elements, $is_root_call = FALSE) {
+    return drupal_render($elements, $is_root_call);

We don't need it. it is a service now.

Thanks! Going to address it.

I'm fine with that change in the Url handler.

In general I really like getting rid of the 'html' in that many places, though I'm not 100% sure
about removing the feature, but well, in case we really need it, we can still add it back, if needed.

+++ b/core/includes/theme.inc
@@ -663,7 +660,7 @@ function template_preprocess_links(&$variables) {
+        '#title' => is_array($link['title']) ? drupal_render($link['title']) : SafeMarkup::escape($link['title']),

Ideally we should not introduce drupal_render(), but this is alright, its not critical for itself.

I replaced the wrapped drupal_render() with the injected renderer service. Was stuck for a long time when I was adapting the PHPUnit tests due to a puzzling problem in which PHP was not making a copy of the $text variable in the render() method when it was overwritten. This is probably a bug in PHP. I even checked in 5.4.17, 5.5.16 and 5.6.2 and 5.6.3, but could replicate it in all PHP versions. I had to resort to renaming the variable in the end:

    // Make sure the link text is sanitized.
-    $text = SafeMarkup::escape($variables['text']);
+    $safe_text = SafeMarkup::escape($variables['text']);

-    return SafeMarkup::set('' . $text . '');
+    return SafeMarkup::set('' . $safe_text . '');

I also fixed a bug in my previous patch, if a dynamic path was starting with a slash it was throwing an error. I also added a test case for this.

Wow, fascinating.

I tried to reproduce it on 3v4l.org, but no dice:

http://3v4l.org/UAkFP

@Fabianx, can you replicate it with the LinkGeneratorTest? Using your minimal script on 3v4l.org I also cannot replicate it. It might be due to the renderer being mocked in the test.

This final thing is just such a nitpick that I don't want to not RTBC just for that.

1. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -135,9 +143,10 @@ public function generate($text, Url $url) {
   +    // Make sure the link text is sanitized.
   +    $safe_text = SafeMarkup::escape($variables['text']);
   +
   +    return SafeMarkup::set('<a href="' . $url . '"' . $attributes . '>' . $safe_text . '</a>');
   

   Renaming the variables is sure fine, given that it makes the code a bit more explicit.

2. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -27,6 +28,22 @@ class ImageFieldDisplayTest extends ImageFieldTestBase {
   +   *
   +   * @var \Drupal\Core\Utility\LinkGenerator
   +   */
   
   +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
   @@ -34,11 +35,20 @@ class ResponsiveImageFieldDisplayTest extends ImageFieldTestBase {
   +   * The link generator.
   +   *
   +   * @var \Drupal\Core\Utility\LinkGenerator
   +   */
   +  protected $linkGenerator;
   

   Final nitpick: Let's typehint the interface here ...

3. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
   @@ -344,10 +353,28 @@ public function testGenerateWithHtml() {
   +      // Mark the HTML string as safe at the moment when the mocked render()
   +      // method is invoked to mimic what occurs in render(). We cannot mock
   +      // static methods.
   +      ->will($this->returnCallback(function () use ($html) {
   

   It is great that we describe why we do things here.

Ahh the drop is on the move again, the patch doesn't apply any more. Will reroll and fix the type hint.

Rerolled and unpicked nits. Only documentation changes so if this is green this is RTBC again.

RTBC as per #153

Looks like we need a change record for this. And we need to link to the original conversion l() too.

Will write change record.

Noticed something while looking at the patch while writing the change record:

+++ b/core/modules/book/src/Tests/BookTest.php
@@ -207,24 +207,34 @@ function checkBookNode(EntityInterface $node, $nodes, $previous = FALSE, $up = F
-      $url->setOptions(array('html'=> TRUE, 'attributes' => array('title' => t('Go to parent page'))));
+        $url->setOptions(array('attributes' => array('title' => t('Go to parent page'))));
       $this->assertRaw(\Drupal::l('Up', $url), 'Up page link found.');

Indentation is wrong here.