HtmlResponseAttachmentsProcessor::processHtmlHeadLink produces invalid HTTP Link header

IMHO bug fixes are pointless without tests :)

+++ b/core/modules/node/src/Tests/NodeViewTest.php
@@ -34,6 +35,27 @@ public function testHtmlHeadLinks() {
+    $expected = [];
+    foreach ($node->uriRelationships() as $rel) {
+      $expected[] = '<' . Html::escape($node->url($rel)) . '>; rel="' . $rel . '"';
+      if ($rel == 'canonical') {
+        // Set the non-aliased canonical path as a default shortlink.
+        $expected[] = '<' . Html::escape($node->url($rel, ['alias' => TRUE])) . '>; rel="shortlink"';
+      }

For sure, this tests the functionality but it also feels like just a reimplementation of the actual code

yeah that feels better, if you ask me.

Rerolled the patch since 2585899-9.patch won't apply to Drupal 8.2.x

Some of the header links in the current patch's tests have been removed since the patch was created. See #2406533: edit-form, delete-form etc. <link> tags added on /node/{node} are invalid according to W3C Validator.

Rerolled without those links, tests should now pass. Removed the "Needs tests"-tag.

+++ b/core/modules/node/src/Tests/NodeViewTest.php
@@ -64,6 +66,24 @@ public function testHtmlHeadLinks() {
   /**
+   * Tests the Link header.
+   */
+  public function testLinkHeader() {
+    $node = $this->drupalCreateNode();

In an ideal world we would have a test which is more direct, aka. not be part of node module, as well, node module is just one of the users of the system

Thanks @dawehner, added additional test coverage to the already existing HtmlResponseAttachmentsTest. No interdiff since the patch contains additional test coverage, no other changes were made to the previous patch.

I've tested the patch from #19 against 8.2.1 and it works properly.

Thank you for addressing that test location problem!

Looks good to my browser/me

+++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
@@ -419,9 +419,13 @@ protected function processHtmlHeadLink(array $html_head_link) {
+          $href .= ';' . $param;

I think for consistency this should be $href .= '; ' . $param;

Because of

function drupal_http_header_attributes(array $attributes = array()) {
  foreach ($attributes as $attribute => &$data) {
    if (is_array($data)) {
      $data = implode(' ', $data);
    }
    $data = $attribute . '="' . $data . '"';
  }
  return $attributes ? ' ' . implode('; ', $attributes) : '';
}

Everything else looks great.

@alexpott: drupal_http_header_attributes() already prefixes its return value with a space, so adding an additional one would result in 2 spaces between the link and the first param:

Link:</node/1>;  rel="shortlink"

Very glad to see this being fixed!

Patch #19 applies cleanly to our system. HtmlResponseAttachmentsTest passes on my local which runs on Apache. On our dev, which runs on nginx, HtmlResponseAttachmentsTest has 8 fails. At HTTP response header X-Drupal-Cache with value MISS (or HIT) found, actual value: neither MISS nor HIT is printing. Where do I look for MISS or HIT in the Response Headers?

Back to RTBC per #25.

#27: that sounds like a local environment issue.

Thanks for responding to my concern @mr.baileys - and showing me it was unfounded.

Committed and pushed e24749a to 8.3.x and 5863276 to 8.2.x. Thanks!

Minor code style fix on commit:

diff --git a/core/modules/system/tests/modules/render_attached_test/src/Controller/RenderAttachedTestController.php b/core/modules/system/tests/modules/render_attached_test/src/Controller/RenderAttachedTestController.php
index 0e764f7..013f100 100644
--- a/core/modules/system/tests/modules/render_attached_test/src/Controller/RenderAttachedTestController.php
+++ b/core/modules/system/tests/modules/render_attached_test/src/Controller/RenderAttachedTestController.php
@@ -82,4 +82,5 @@ public function htmlHeaderLink() {
     $render['#attached']['html_head_link'][] = [['href' => '/foo/bar', 'hreflang' => 'nl', 'rel' => 'alternate'], TRUE];
     return $render;
   }
+
 }

Since this is a bug, I think it should also be cherry-picked to 8.2.x?

@Wim Leers it's there :) http://cgit.drupalcode.org/drupal/commit/?id=5863276

Oh weird, it's not showing up in this issue, only the 8.3.x commit is showing up, in #30 :(

Sorry!

I'm seeing the commit in the 8.2.x branch, but not in drupal-8.2.2.tar.gz

@esod it'll be in the next patch release of 8.2.x - that's 8.2.3 - 8.2.2 was released before this change was committed.