HTML head has alternate hreflang links to unpublished translations

I tested patch in #1 and looks like it fix the problem

+++ b/core/modules/content_translation/content_translation.module
@@ -554,6 +554,13 @@ function content_translation_page_attachments(&$page) {
+        // Skip any unpublished node translation.
+        if ($entity instanceof \Drupal\node\Entity\Node) {
+          $translation = $entity->getTranslation($language->getId());
+          if (!$translation->isPublished()) {
+            continue;
+          }
+        }

Instead of a hardcoded check for nodes, why not check for access for each translation? That should result in the same and work for everything.

Hmm yeah, calling $translation->access('view') will return true if the original entity has access, because in Node.php prepareLangcode() is looking for the current language.

Attached patch fixes that, but not sure if this is the right approach as it's technically an API change.

Note, that the isPublished() check might be a go to fix this patch, but I think there's a still a bug with calling $translation->access('view')

Thanks for providing a patch, but in the current form it is dealing only with nodes, but it should support any content entity type instead. For instance:

+++ b/core/modules/content_translation/content_translation.module
@@ -562,6 +562,13 @@ function content_translation_page_attachments(&$page) {
+        // Skip any unpublished node translation.
+        if ($entity instanceof \Drupal\node\Entity\Node) {
+          $translation = $entity->getTranslation($language->getId());
+          if (!$translation->isPublished()) {
+            continue;
+          }
+        }

We should check $entity->access() here.

1. +++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
   @@ -371,26 +381,38 @@ protected function doTestTranslations($path, array $values) {
   +   * Tests that the path for the given node provides the correct alternate hreflang links.
   

   The first PHP doc line should not exceed 80 chars.

2. +++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
   @@ -371,26 +381,38 @@ protected function doTestTranslations($path, array $values) {
   +   * @throws \Drupal\Core\Entity\EntityMalformedException
   

   Missing blank line separator before this and the description of when the exception is thrown.

+++ b/core/modules/content_translation/content_translation.module
@@ -566,6 +566,13 @@ function content_translation_page_attachments(&$page) {
+        if ($entity instanceof \Drupal\node\Entity\Node) {

This check should go away, after that it's good to go.

Looks good to me now. My patches are completely unrelated, so RTBC

+++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
@@ -372,26 +382,39 @@ protected function doTestTranslations($path, array $values) {
+            $this->assert(isset($links[0]), format_string('The %langcode node translation has the correct alternate hreflang link for %alternate_langcode: %link.', array('%langcode' => $langcode, '%alternate_langcode' => $alternate_langcode, '%link' => $url->toString())));

This is deprecated, but I see it's just been copied from the existing test, so committing as is to 8.1.x and 8.0.x, thanks!

+++ b/core/modules/content_translation/content_translation.module
@@ -566,6 +566,11 @@ function content_translation_page_attachments(&$page) {
+        // Skip any translation that cannot be viewed.
+        $translation = $entity->getTranslation($language->getId());
+        if (!$translation->access('view')) {
+          continue;
+        }

This should set the access result's cacheability metadata on $page.

The reason tests pass is probably that by default access is granted based on permissions, and user.permissions is a required render cache context.

Good point. Rolled back for now.

I see the status is 'needs work' but reading through the comments it isn't clear to me what exactly remains to be done.. what's the status / what needs to be done to get this committed?

Same question as above. I see the fix has been committed to 8.3.x but still needs work for 8.2.x?

It is comment #26. And it was reverted in 8.3.x and 8.2.x.

Yup, the path from #26 was committed and then rolled back - that's why I was asking what actually remains to be done, since the status is "Needs work".

From #26: "The reason tests pass is probably that by default access is granted based on permissions, and user.permissions is a required render cache context."
I'm not really following this and would need to dive into the code to catch up.. am hoping some of the earlier contributors and provide some explanation of status.

#26 is the reason it was rolled back.

@cilefen I get that... but what is lacking here is a summary of what specific action items still have to be done in order to get this patched on the 8.2 branch. I don't know what those steps would be.

Is the question about what steps are required to accomplish the review items in #26?

Yes. It says;
'This should set the access result's cacheability metadata on $page.'

The reason tests pass is probably that by default access is granted based on permissions, and user.permissions is a required render cache context.'

I can't tell whether this means that content_translation.module is OK, or whether there's a bug in the tests, or both. Sounds like the tests pass but shouldn't, i.e. the module code is not in fact setting cacheability, even though tests say that it is. Would be great if someone could confirm and explain in a bit of detail.

Considering there hasn't been any change in the past 5 months, I've removed the assignment to @paulmckibben.

I assume @wim-leers meant that we should add the cache metadata to the $page render array. I've attached a patch that should add that caching information. Whether the tests need to be updated or not is beyond me, though.

#45: great! Thanks for getting this going again :) The patch looks good! But it'll need to bring back the test coverage changes that #21 was adding.

I merged the simple test from #21 with the last patch from #45 and tested the Simple Test locally. Prepared it in its own patch.

His solution, worked well ! Well done!

I was going to rtbc this, but I noticed some whitespace that should be removed. Sorry for the nitpick.

+++ b/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
@@ -374,24 +374,35 @@ protected function doTestTranslations($path, array $values) {
+   */ ¶

Whitespace should be removed here.

Changes done as per comment #52 & also added an interdiff.

Patch doesn't apply anymore and introduces some cs errors. ([] should be used instead of array()).

I just tested drupal-alternate_hreflang_unpublished_with_test-2521782-54.patch on it applied cleanly for me on dev and 8.5.0. Tested and working on 8.5.0 for me but I have not tested it on dev specifically. I've re-rolled it with array() converted to [].

Re-rolled

Here is an only reroll to 8.6.x

Here is an only reroll to 8.7.x

I guess that some stuff from #2994800: Improve the alternate hreflang implementation shoud be considered.

I performed the following from module_name_preprocess_html():

var_dump($variables['page']['content']['system_main']['#attached']['html_head_link']);

I found the rel="canonical", rel="shortlink" and rel="revision" tags. I was not able to find the rel="alternate" hreflang= even though the resulting page contains three such tags -- one for en, es and zh-hans.

Should hreflang be part of the $variables array as described above? Is there another way to access the hreflang tags from module_name_preprocess_html()? I would like to be able to unset() an hreflang that points to an unpublished translation.

It appears that updating the language code in $variables has no impact on the creation of the associated hreflang tag.

I found the hreflang tags here:

var_dump($variables['page']['#attached']['html_head_link']);

I changed the language code for Chinese to zh according to the standard:

  [2]=>
  array(2) {
    [0]=>
    array(3) {
      ["rel"]=>
      string(9) "alternate"
      ["hreflang"]=>
      string(2) "zh"
      ["href"]=>
      string(53) "https://www.raystedman.org/zh/new-testament/ephesians"
    }
    [1]=>
    bool(true)
  }

This resulted in the following hreflang tag on this page:

<link rel="alternate" hreflang="zh-hans" href="https://stg.raystedman.org/zh/new-testament/ephesians" />

Please notice that the hreflang value does not conform to the value in $variables.

The patch on #62 fails on latest version of 8.7.x

Here is a re-roll for 8.7.x

The patch in #67 was not applying correctly for me on the latest version of 8.7.x, so I re-reolled it

Looks like the test failed on 8.8.x because the call to \Drupal\Core\Entity\EntityInterface::urlInfo() in NodeTranslationUITest became deprecated in 8.8.x

diff --git a/core/modules/node/tests/src/Functional/NodeTranslationUITest.php b/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
index 3348ad9d90..d870cfe39c 100644
--- a/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
+++ b/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
@@ -378,24 +378,35 @@ protected function doTestTranslations($path, array $values) {
   /**
    * Tests that the given path provides the correct alternate hreflang links.
    *
-   * @param \Drupal\Core\Url $url
-   *   The path to be tested.
+   * @param \Drupal\node\Entity\Node $node
+   *   The node to be tested.
    */
-  protected function doTestAlternateHreflangLinks(Url $url) {
+  protected function doTestAlternateHreflangLinks(Node $node) {
+    $url = $node->urlInfo();

... I'll look into fixing this.

Looks like the patch needs to be re-rolled as well; and the re-roll onto 8.8.x and 8.9.x is complex because some other functions (e.g.: format_string()) have been deprecated and replaced, leading to merge conflicts.

This means I'm going to have to re-roll this in stages.

Attached is a re-roll of the patch from #69 onto the HEAD of 8.7.x at time-of-writing, commit 3fe90fe545. I haven't made any changes yet, so no interdiff is necessary.

I fully expect this to fail to patch on 8.8.x and 8.9.x so I'm going to leave the issue as "Needs work" for Testbot's sake.

Here's a patch where I've replaced calls to format_string() with calls to new FormattableMarkup() as per https://www.drupal.org/node/2302363

I was hoping the change in #73 would allow git to rebase the patch onto 8.9.x automagically, because the removed lines would be the same as the lines in the patch; but git still presented me with a merge conflict in NodeTranslationUITest.php. Although, to resolve the merge conflict, I could simply delete the old code - I didn't need to make any changes to the code in the patch.

So here's the patch from #73 rebased onto 8.9.x - since there were no changes, there's no interdiff.

Still "needs work" until I can fix the deprecated use of EntityInterface::urlInfo()

Oops: I did change urlInfo() to toUrl() in that patch; likely while I was trying to figure out why Git didn't want to rebase the patch without a merge conflict.

That was the only change; although generating an interdiff is not possible since it would include all the changes from 8.7.x to 8.9.x as well.

So I'm setting this to "needs review" so Testbot can try it out.

Sorry for the noise and confusion.

Yay; tests are green again!

Using this in production and it works great.

@weseze, if you've tested it (i.e.: in production), and reviewed the patch (i.e.: by reading through it for problems), may I trouble you to change the status to "Reviewed & tested by the community"? (I cannot do that myself because I wrote the patch) Thank you!

If this gets merged on July 2 it will be merged on the fifth year anniversary of the patch!

Committed/pushed to 9.1.x and cherry-picked back to 8.9.x, four years after the original fix was reverted but not quite in time to hit the five year birthday.

Hi, what if only one language is available? It looks that currently there is always at least one hreflang tag printed. Is this ok?