Menu item translation : breadcrumb displays (twice) "add" that have no sense

Basically the Add was an attempt to create the link to the same page where they were shown, but that broke in 2 links (something like add/en/es is the right link and it created add/ and then add/es) The main question here is why are those added and if this may happen in some other place.

This happens for any translation add screen, also for content for instance, so I guess it simply happens everywhere.

Tagging and re-categorizing.

Closing #2656172: Fatal errors can be triggered on content translation add routes (+ incorrect breadcrumb on add form) as a duplicate.

Like I proposed in the duplicate issue, I think we should also remove the NULL language from the edit and delete route.

And we need tests for this.

Added a test and whitelisted links in NodeViewController.

Yeah I had already found that problem with NodeViewController in #2656172: Fatal errors can be triggered on content translation add routes (+ incorrect breadcrumb on add form), but forgot to mention it here.

The test looks good. What is not being tested is the bogus breadcrumb that is there. So marking "Needs work" for that.

Also maybe we can tackle the edit and delete routes here, as I mentioned in #10, but I'm also fine with doing that in a follow-up.

Added a test for the breadcrumb links, removed the NULL language from the edit and delete routes and changed the content translation access check to perform the access checks against the unchanged entities from storage.

+++ b/core/modules/node/src/Controller/NodeViewController.php
@@ -22,6 +22,11 @@ public function view(EntityInterface $node, $view_mode = 'full', $langcode = NUL
+      }

Hmm, we should document this somehow for other entity types out there. There's no other way we can control this more upstream in the code ?

Fixed ContentTranslationManageAccessCheckTest.

Re @swentel #20: Yeah, this is not the nicest code out there. I discussed this with @morenstrat in Person.

I personally found blacklisting the content translation links instead of the whitelisting even more hacky, that's why I suggested this approach.

The current code is certainly broken (i.e. even in HEAD). Its just that this patch exposes the broken links.

In General our "API" regarding entity links is very lacking, i.e. there is no way to distinguish e.g. canonical from content-translation-add.

So yeah, not sure about this, but also not sure what else to do...

Looked at the patch now. I have nothing to complain; this looks good to go to me. Because this touches a couple of tricky parts, though, not setting RTBC yet but waiting for another set of eyes, especially an answer from @swentel

I don't have any better idea now either, so I won't hold this off. Feel free to RTBC :)

OK, coolio

+++ b/core/modules/node/src/Controller/NodeViewController.php
@@ -22,6 +22,11 @@ public function view(EntityInterface $node, $view_mode = 'full', $langcode = NUL
+      // Whitelist links to remove content translation links.

I know it's been discussed a bit, but would like to figure out if this is really the only way to do it.

What about other links not in the list that might be added? Do we know they should always be removed too?

Why can't content_translation alter its own links out of here? Then we'd need neither a blacklist nor a whitelist.

Mmhh.. There's no alter hook that gets called because this directly returns the controller result. What we could do is add a #pre_render callback in hook_entity_view() which then does the unsetting for us. I'm going to try that.

Thanks all for your work here.
This issues becomes a bit hard to follow. Could you move that links removal in another issue so we can focus on breadcrumbs here and get it fixed quicker?

@DuaelFr: I would, if it were possible. The links that are generated in HEAD are broken already, it's just that it happens to go through without a fatal coincidentally. By fixing the bogus optionality of the arguments we are exposing that bug, by actually making the link generation fail. Thus, both bugs need to be fixed together in order to get the tests to pass.

@tstoeckler Ok so you need to change the issue title and summary to reflect that fact :)
Thank you for your enlighting answer!

True! Thanks for noting that.

@tstoeckler: are you sure we can fix this in a pre render callback? The exception is thrown when the content translation add url is added to the render array.

I'm not sure about "allowed" changes in patch or minor releases. Would it be an option to add an alter hook to Entity::uriRelationships() so that the content translation module could remove its links?

@morenstrat: He, you are right, good catch. I should really know better by know than to simply claim that something is possible without trying it out... Because, generally we now have Url objects, that only get rendered when we actually hit the Twig template I thought this could actually work, but it doesn't at least not just like that.

The problem: NodeViewController and the two other places this code was copy-pasted to in core - NodePreviewController and taxonomy_page_attachments_alter() - just use $entity->url() which converts to a string directly, instead of (properly) using $entity->toUrl() which returns such a Url object. So if we change that, we can then implement hook_entity_build_defaults_alter() in Content Translation to add a #pre_render callback to the build that removes the respective links from $build['#attached']['html_head_link']. (Note that despite what I said above, I did not actually try this last part out, so beware... ;-))

...except then we hit another problem: In this particular case the Url objects don't just end up being rendered by Twig templates (which would work fine) because that would be just too easy... Instead HtmlResponseAttachmentsProcessor converts these into render placeholders but it does not account for the href key to be a Url object. I think it is in-line with our philosophy of "rendering as late as possible" to make HtmlResponseAttachmentsProcessor convert Url objects to strings if it encounters them, so after spending some quality time trying out a variety of different places to stick these 3 lines, I came up with the following "patch" for \Drupal\Core\Render\HtmlResponseAttachmentsProcessor::processHtmlHeadLink() which then should make the code suggested in the previous paragraph actually work:

  protected function processHtmlHeadLink(array $html_head_link) {
    $attached = [];

    foreach ($html_head_link as $item) {
      $attributes = $item[0];
      $should_add_header = isset($item[1]) ? $item[1] : FALSE;

+     if ($attributes['href'] instanceof Url) {
+       $attributes['href'] = $attributes['href']->toString();
+     }

      $element = array(
        '#tag' => 'link',
        '#attributes' => $attributes,
      );
      $href = $attributes['href'];
      $attached['html_head'][] = [$element, 'html_head_link:' . $attributes['rel'] . ':' . $href];

      if ($should_add_header) {
        // Also add a HTTP header "Link:".
        $href = '<' . Html::escape($attributes['href'] . '>');
        unset($attributes['href']);
        $attached['http_header'][] = ['Link', $href . drupal_http_header_attributes($attributes), TRUE];
      }
    }
    return $attached;
  }

... however, I'm not sure that's actually such a good idea - despite the late rendering (which in itself is a good thing) because $url->toString() looses cacheability metadata of the URL generation. In NodeViewController (and the other places) where this is currently being called (because $entity->url() which is actually being called is just a shorthand for $entity->toUrl()->toString) we are operating inside of the render context of the main page controller so the metadata does actually get caught in the end. In HtmlResponseAttachmentsProcessor, however, we are not in any render context as far as I know. So on top of increasing the complexity of the patch a fair bit, pursuing this direction would actually be a regression of the cacheability metadata, unless I'm missing something.

Due to all this assigning to @catch for some feedback on how we want to proceed here. I sincerely hope I am making any sense here; I apologize for being a little bit ramble-y. It's fairly late here and I just spent an amount of time larger than I'd like to admit staring at HtmlResponseAttachmentsProcessor...

And thanks again @morenstart for setting me straight!

Re #34: I guess it really depends on the semantics of the uriRelationships() method whether something like that would be appropriate. Content Translation only adds those link-templates in the first place because they are used for the route generation of the content translation routes. So we want Content Translation to show up in the link templates (at least in those specific cases), and since currently $entity->uriRelationships() is literally array_keys($entity->linkTemplates()) it shows up in there as well. So I don't really know how to fix this "properly", to be honest. An alter hook might be a solution, but I'm not sure of all the consequences of this.

There is #2113345: Define a mechanism for custom link relationships which aims to provide more of a proper API for link relations (rather than the sort of "free-for-all dumping ground" we have now) but that has seen no activity in almost a yearand has no actual code... :-/

Maybe @catch or someone else who is more knowledgeable about the whole area of link relations can provide some input here.

Because I still had this applied locally and played around with the site for some other purpose I just found another tidbit that we would need to adress if we were to pursue #27 and later:
I said that $entity->url() is equivalent to $entity->toUrl()->toString() and did not mention the fact that the former actually does a check for $entity->id() and simply returns an empty string if there is no ID, because I thought this check is not relevant.

It actually is, however, we currently rely on it in order for the link-generating code to not fatal. So we would have to adapt (at least) NodePreviewController for that, as well. Although, thinking about it more, I'm not sure why we even generate those links for a preview. Unless I'm missing something this seems incredibly pointless.

Opened #2751501: NodePreviewController should not render entity links in the HTML head for the point noted in #37.

This is an attempt at resolving this issue by avoiding the code mentioned by @catch in #27 and instead hardcoding knowledge about the Content Translation routes in Entity::uriRouteParameters() - also not very elegant, but completely backwards compatible as far as I know.

Note that in order to provide test coverage for the new code in Entity::uriRouteParameters() this would have to be postponed on #2751395: Rewrite EntityUrlTest, but not marking as such yet to get some feedback on the approach first.

Just shooting in the dark here.

Okay, only ever so slightly less elegant. Let's see if this is green.

Now that #2751395: Rewrite EntityUrlTest is in, this could use some test coverage. Still leaving at Needs review, though, for some input on the general direction.

Decided to remove the complex language finding - it's pointless to call this anyway. Added a comment to that extent as well. Also adds some tests.

Patch naming fail.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -12,6 +12,7 @@
+use Drupal\Core\TypedData\TranslatableInterface;

@@ -310,6 +311,19 @@ protected function urlRouteParameters($rel) {
+    // Nodes and taxonomy link to all possible link relations in the HTML head,
+    // so we need to support calling $entity->toUrl() for the content
+    // translation link relations, even though that does not make sense
+    // semantically without being able to specify the translation language.
...
+    elseif ($rel === 'drupal:content-translation-add' && $this instanceof TranslatableInterface) {
+      $uri_route_parameters['source'] = $this->language()->getId();
+      $uri_route_parameters['target'] = $this->language()->getId();
+    }
+    elseif (in_array($rel, ['drupal:content-translation-edit', 'drupal:content-translation-delete'], TRUE) && $this instanceof TranslatableInterface) {
+      $uri_route_parameters['language'] = $this->language()->getId();
+    }
+

Remove those lines, because node/2113345 was fixed.

Reroll. This patch work for me.Thanks.

Just reroll.

Here a rebase of #55 for Drupal 8.9.x.

(This might still needs work but I want to know how many tests it is going to break)

Lets try again.

Rerolled for 9.3.x

Fixed the coding standard issues.

Re-rolled the patch for 9.5.x

+++ b/core/lib/Drupal/Core/Entity/EntityBase.php
@@ -275,6 +275,19 @@ protected function urlRouteParameters($rel) {
+    // @todo Remove this after https://www.drupal.org/node/2113345

issue is fixed

Yes, #2113345: Define a mechanism for custom link relationships has been fixed on January, 2017. That comment is not necessary any more.

Create a patch for drupal 10.1.x