content_translation_page_attachments() should check for a canonical link before generating a URL to it

Re-uploading for the bots.

+++ b/core/modules/content_translation/content_translation.module
@@ -523,10 +523,10 @@ function content_translation_page_attachments(&$page) {
       foreach ($entity->getTranslationLanguages() as $language) {
-        $url = $entity->urlInfo()
+        $url = $entity->urlInfo('canonical')
           ->setOption('language', $language)

'canonical' is the default value, no need to add it, also I suggest to change it to toUrl() as urlInfo() is deprecated.

So I'm providing a patch with this done.

This error breaks some contrib modules like courier and all based on it.

reroll for current 8.2.x

This is affecting Drupal Commerce where entities do not have canonical routes #2826693: Content translation breaks the promotion forms.

Also affects Entityqueue which recently removed its canonical link.
Subqueues should not have a canonical link in order for them to be exportable through REST

Confirming patch in #8 prevents an error from being thrown.

I can confirm that the issue with EntityQueues is fixed with the patch in #8.

+++ b/core/modules/content_translation/content_translation.module
@@ -572,10 +572,10 @@ function content_translation_page_attachments(&$page) {
+        $url = $entity->toUrl()

I think it's better to pass 'canonical' as an explicit parameter like @tstoeckler did in the initial patch, it makes it easier to understand the hasLinkTemplate() check above.

Testing ....

I confirm that patch in #8 fixed the problems here. I dont know if it's better or not to pass 'canonical' as parameter since toUrl() method have 'canonical' as default value and seems like its redundant. So, the patch in #8 looks fine to me.

Patch in #8 fixed the issue.

I confirm. The issue is fixed now with the latest patch

Added test coverage for link tags.

Restored the explicit rel which was removed in #6, I agree with #1 it should remain. Other than that this new patch is exclusively new tests:

• Ensure no canonical links are present for entity type without canonical link. Found in \Drupal\Tests\content_translation\Functional\ContentTranslationLinkTagTest::testCanonicalAlternateTagsMissing.
• The only existing coverage for the link tags is found in the node-specific \Drupal\node\Tests\NodeTranslationUITest::doTestAlternateHreflangLinks. The new test found in \Drupal\Tests\content_translation\Functional\ContentTranslationLinkTagTest::testCanonicalAlternateTags uses the generic entity test entities.

Tests only

queued retests, I think patch should go into 8.4 first and then backported to 8.3 & 8.2

Fixed PHP syntax issue. Apparently I was using a PHP 7 only feature without knowing it.

Interdiff with #17

The test only patch in #23 should not have passed. During cleanup I removed the a permission, which caused the assert-missing to erroneously pass on an access denied page. Fixed in this patch by adding correct permission, and checking HTTP response code.

Thanks for the review @tstoeckler!

array to string[]

Done

Super minor but would it make sense conceptually to move this into setupLanguages()?

Done

This seems a bit unnecessarily complex to first add the default langcode and then remove it for that again when using it here.

Considering the variable name could be considered to be all languages on the site I thought it made sense. Taken feedback into account, changed it to be added languages only.

kind of unnecessary IMO to first iterate over the langcodes and set the languag on the URL and then fetch the langcode again a couple lines below.

I considered adding keys to $urls. The langcodes were already in the objects, keeping it functional.

Made the change anyhow.

This is exactly how tests should be written

Thanks!

Seems the setAbsolute() could be moved to $url_base, right?)

Done

We could also just iterate over the langcode directly and then clone and set the language in that foreach.

Keep in mind I am iterating over $urls twice, to get a multidimensional test. (Nested foreach ($urls as $url) {), so I need to set the URLs beforehand. Otherwise it gets messy having to create URLs 9 times, instead of 3. ...I could have misinterpreted this comment.

Interdiff is vs #23

This test only patch should fail.

Great!

Testbot looks good.

1. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationLinkTagTest.php
   @@ -0,0 +1,154 @@
   +  /**
   +   * The language manager.
   +   *
   +   * @var \Drupal\Core\Language\LanguageManagerInterface
   +   */
   +  protected $languageManager;
   +
   +  /**
   +   * The entity type manager.
   +   *
   +   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   +   */
   +  protected $entityTypeManager;
   ...
   +    $this->languageManager = $this->container->get('language_manager');
   +    $this->entityTypeManager = $this->container->get('entity_type.manager');
   +    $this->setupUsers();
   +    $this->setupLanguages();
   ...
   +    // Rebuild the container so that the new languages are picked up by services
   +    // that hold a list of languages.
   +    $this->rebuildContainer();
   

   Rebuilding the container after attaching services to the test is a bit dangerous - if the rebuild changed these services this wouldn't work. Best thing here is to not attach the services to protected class properties at all.

2. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationLinkTagTest.php
   @@ -0,0 +1,154 @@
   +  protected function setupLanguages() {
   ...
   +  protected function setupUsers() {
   

   Imho these methods should be be inlined into ::setUp()... it's a level of indirection that doesn't help test understandability.

3. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationLinkTagTest.php
   @@ -0,0 +1,154 @@
   +  public function testCanonicalAlternateTags() {
   ...
   +  public function testCanonicalAlternateTagsMissing() {
   

   Great to see complete testing of the problem space.

Addressed feedback from #29

these methods should be be inlined into ::setUp()... it's a level of indirection that doesn't help test understandability.

No problem, these were remnants of abstractions originating from its sister web tests extending \Drupal\content_translation\Tests\ContentTranslationTestBase.

Interdiff with #25

feedback addressed, should be ported to 8.3

Committed and pushed cd8293e to 8.4.x and 94389e7 to 8.3.x. Thanks!

As this is a bug fix and makes n o BC implicating changes backporting to 8.3.x immediately.