Automate link relationship headers for all entity types, not just nodes

This is as straight a rebase as is possible.

--- a/core/modules/node/node.routing.yml
+++ b/core/modules/node/node.routing.yml
@@ -47,7 +47,7 @@ node.add:
 node.view:
   path: '/node/{node}'
   defaults:
-    _content: '\Drupal\node\Controller\NodeViewController::view'
+    _entity_view: 'node.full'
     _title_callback: '\Drupal\node\Controller\NodeViewController::title'
   requirements:

This change is omitted in this rebase, since this route is now defined in NodeRouteProvider and will require additional changes.

See next comment/patch.

Better title.

So the route change mentioned in #8 can no longer be done in the same way. That route is now being defined in \Drupal\node\Entity\NodeRouteProvider::getRoutes() and looks like this:

    $route = (new Route('/node/{node}'))
      ->addDefaults([
        '_controller' => '\Drupal\node\Controller\NodeViewController::view',
        '_title_callback' => '\Drupal\node\Controller\NodeViewController::title',
      ])
      ->setRequirement('node', '\d+')
      ->setRequirement('_entity_access', 'node.view');

So we can't do what this patch is doing: move away from NodeViewcontroller::view() to _entity_view: 'node.full'. Or can we?

Turns out we can!

• Turns out NodeViewController::title() is doing the functional equivalent of the default title callback that \Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider::getCanonicalRoute() sets: \Drupal\Core\Entity\Controller\EntityController::title() does effectively the same.
• Turns out that NodeViewController::view() just calls EntityViewController::view() … and then just adds the link relations. That's all it does.

In other words: it's trivial to move the link relation logic out of NodeViewController and into EntityViewController… or potentially a subscriber, like this patch is currently doing.

And in fact, simply moving the logic from NodeViewController to EntityViewController seems to be the most prudent approach. It means no extra overhead (unlike an event subscriber). It means less BC break (because $build['#attached'] still contains the links). And it's the simplest approach.

---

So this:

1. makes NodeRouteProvider extend DefaultHtmlRouteProvider
2. moves the logic from NodeViewController to EntityViewController

core/modules/node/src/Controller/NodeViewController.php can now simply be removed.

The link_relationship_subscriber service definition and its code (core/lib/Drupal/Core/EventSubscriber/LinkRelationshipSubscriber.php) can now also simply be removed.

UserRouteProvider also isn't inheriting from DefaultHtmlRouteProvider, and should be. It'll then be consistent with NodeRouteProvider.

And then some final cleanup.

This still needs tests though.

Or am I misunderstanding which controller we're dealing with here?

Yes, you are :)

You're confusing EntityViewController (which is a controller used only for the canonical route) with EntityViewBuilder (which is used to render (build a view of) an entity, for every place that entity may be displayed).

Apparently it's taxonomy_page_attachments_alter() that's adding link relation headers for the canonical Term route! We'll need to refactor that away here too.

Well there's nothing to move, it was doing exactly the same as NodeViewController, and that now lives in EntityViewController, so it was already working for taxonomy terms :)

This should fix some of the test failures.

And this should fix the remainder.

People already complained hugely about those header links being there. They seem to have huge SEO issues about that. Given that it feels like these links should be enableable, but maybe disabled by default.

One way to solve that could be its own module for that. Otherwise maybe some configuration per entity type could be done.

They seem to have huge SEO issues about that.

That was because they were all exposed always, even to anonymous users. Thanks to #2406533: edit-form, delete-form etc. <link> tags added on /node/{node} are invalid according to W3C Validator, that's no longer the case. :)

@Wim Leers I was looking into the test failure in \Drupal\system\Tests\Entity\EntityViewControllerTest::testEntityViewController
$this->assertRaw($get_label_markup($entity->label()));
I think the problem is

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -103,13 +103,13 @@ public function buildTitle(array $page) {
+    $page['entity']['#pre_render'][] = [$this, 'buildTitle'];
...
+    $page['entity']['#' . $page['entity']['#entity_type']] = $_entity;
 

I think \Drupal\Core\Entity\Controller\EntityViewController::buildTitle also needs to be update to match this new nesting. I tried to figure this out but

if (isset($page[$label_field])) {
  $page['#title'] = $this->renderer->render($page[$label_field]);
}

Here is $page[$label_field] is not set so this is never called.

If look at \Drupal\entity_test\EntityTestViewBuilder::buildComponents
it adds keys to the build which seems to only happen if the entity is not nested under 'entity' in the page. I can't get these keys to be added otherwise :(

#2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase, #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits and #2864121: Convert web tests to browser tests for statistics module all caused this patch to need a reroll. No interdiff, because functionally, the patch is still identical.

#2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available improved \Drupal\Core\Entity\Entity::uriRelationships(): it now only returns those link relationships for which a URL actually exists for that entity type (i.e. it automatically omits those link relationships that are defined but for which no actual route exists).

That might have fixed some of the failures. If it did not, then it at least made it more reliable.

Here's a fix for all the failures due to

 Uncaught PHP Exception Drupal\Core\Entity\EntityMalformedException: "The "node" entity cannot have a URI as it does not have an ID" at /var/www/html/core/lib/Drupal/Core/Entity/Entity.php line 179

Hopefully somebody else can push it over the finish line! Note that #33 has not yet been addressed.

Looking at #33 agian.

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -91,13 +103,52 @@ public function buildTitle(array $page) {
-    $page = $this->entityManager
+    $page['entity'] = $this->entityManager
       ->getViewBuilder($_entity->getEntityTypeId())
       ->view($_entity, $view_mode);
 
-    $page['#pre_render'][] = [$this, 'buildTitle'];
-    $page['#entity_type'] = $_entity->getEntityTypeId();
-    $page['#' . $page['#entity_type']] = $_entity;
+    $page['entity']['#pre_render'][] = [$this, 'buildTitle'];

Ok this what was causing the problem EntityViewControllerTest::testEntityViewController not being able to find the title on the page.

Shifting
$page['#pre_render'][] = [$this, 'buildTitle'];
down a level to
$page['entity']['#pre_render'][] = [$this, 'buildTitle'];
Means it can't actually affect the title of the page because it won't have $page top level element.

But if you try to set it back to
$page['#pre_render'][] = [$this, 'buildTitle'];
Then the entity won't actually be built yet because

$this->entityManager
       ->getViewBuilder($_entity->getEntityTypeId())
       ->view($_entity, $view_mode);

Actually set a #pre_render callback to EntityViewBuilder::build() originally
$page['#pre_render'][] = [$this, 'buildTitle'];
was setting the #pre_render to run right after
EntityViewBuilder::build()

Because they aren't on the same level any more this is not possible.

I got around this in this patch by build the entity directly in EntityViewController::buildTitle()

This fixed at least 2 test for me locally but it feels heavy handed.

From 15 to 9 failures, great! :)

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -73,16 +74,25 @@ public static function create(ContainerInterface $container) {
+      $view_mode = isset($entity_output['#view_mode']) ? $entity_output['#view_mode'] : 'full';
+      $displays = EntityViewDisplay::collectRenderDisplays([$entity], $view_mode);
+      $builds = [$entity_output];
+      $this->entityManager
+        ->getViewBuilder($entity->getEntityTypeId())
+        ->buildComponents($builds, [$entity], $displays, $view_mode);
+      $entity_build = reset($builds);

Adding all this indeed does feel very heavy-handed…

Is it because we shifted things down a level (under the 'entity' key), and hence the things that buildTitle() was expecting to already been done, have actually not yet been done at this time, and hence you need to do it manually here? (Hence doing much of the work multiple times?)

Node.Drupal\node\Tests\PagePreviewTest
✗	
doGenerate
exception: [Uncaught exception] Line 202 of core/lib/Drupal/Core/Routing/UrlGenerator.php:
Symfony\Component\Routing\Exception\InvalidParameterException: Parameter "node_revision" for route "entity.node.revision" must match "[^/]++" ("" given) to generate a corresponding URL. in Drupal\Core\Routing\UrlGenerator->doGenerate() (line 202 of /var/www/html/core/lib/Drupal/Core/Routing/UrlGenerator.php). Drupal\Core\Routing\UrlGenerator->doGenerate(Array, Array, Array, Array, Array, 'entity.node.revision') (Line: 249)

Hm, this one is interesting. It's because Node has this:

 *   links = {
 *     "canonical" = "/node/{node}",
 *     "delete-form" = "/node/{node}/delete",
 *     "edit-form" = "/node/{node}/edit",
 *     "version-history" = "/node/{node}/revisions",
 *     "revision" = "/node/{node}/revisions/{node_revision}/view",
 *     "create" = "/node",
 *   }

Note how all of those only require {node}… except the revision link template, which requires {node_revision}.

Normally this edge case is taken care of by this code in Entity::toUrl():

    // Links pointing to the current revision point to the actual entity. So
    // instead of using the 'revision' link, use the 'canonical' link.
    if ($rel === 'revision' && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
      $rel = 'canonical';
    }

But, \Drupal\node\Tests\PagePreviewTest::testPagePreviewWithRevisions() does this after testing a "normal" new revision as created through the UI:

    // Check that previewing a forward revision of a node works. This can not be
    // accomplished through the UI so we have to use API calls.
    // @todo Change this test to use the UI when we will be able to create
    // forward revisions in core.
    // @see https://www.drupal.org/node/2725533
    $node->setNewRevision(TRUE);
    $node->isDefaultRevision(FALSE);

    /** @var \Drupal\Core\Controller\ControllerResolverInterface $controller_resolver */
    $controller_resolver = \Drupal::service('controller_resolver');
    $node_preview_controller = $controller_resolver->getControllerFromDefinition('\Drupal\node\Controller\NodePreviewController::view');
    $node_preview_controller($node, 'full');

… and that is something that hasn't been taken into account yet: in that case, you do need {node_revision}. But we can't provide it, because we're previewing an unsaved revision! Therefore generating the revision link relationship in this case does not make sense, and we just need to handle an additional edge case in Entity::toUrl().

This should fix one more failure!

Ok the change in #43 broke UuidFormatterTest because of the way \Drupal\Core\Field\Plugin\Field\FieldFormatter\StringFormatter::viewElements() handles the 'link_to_entity' setting.

 if ($this->getSetting('link_to_entity')) {
      // For the default revision this falls back to 'canonical'
      $url = $items->getEntity()->urlInfo('revision');
    }

I changed to \Drupal\Core\Entity\Entity::toUrl() to first check $this->isDefaultRevision() first before it throws an exception.
Because at that point it should be using 'canonical'

And this should fix the remaining failures (all in NodeCacheTagsTest). The reason it was failing is that NodeCacheTagsTest::getDefaultCacheContexts() used to contain a work-around for the fact that NodeViewController adds a user.roles:anonymous cache context. But it's meant to only apply to the entire entity page… yet it was being applied to the rendered entity (in full view mode) itself! Therefore it was actually doing cache pollution.

That's why it's important that EntityViewController moved the rendering of the entity in full view mode down a level: then the $page['#cache']['contexts'][] = 'user.roles:anonymous'; no longer affects it: cache pollution solved! And hence our tests are passing again, because they were actually asserting this cache pollution, and that's now been fixed :)

Unassigning myself but still need to answer from #43

Is it because we shifted things down a level (under the 'entity' key), and hence the things that buildTitle() was expecting to already been done, have actually not yet been done at this time, and hence you need to do it manually here? (Hence doing much of the work multiple times?)

Yes, it is because it is under the new key 'entity'

\Drupal\Core\Entity\Controller\EntityViewController::view

$page['entity'] = $this->entityManager
      ->getViewBuilder($_entity->getEntityTypeId())
      ->view($_entity, $view_mode);

Actually adds the build() method of Entity View builder to the #pre_render callback.
@see \Drupal\Core\Entity\EntityViewBuilder::view()

When this was the $page level it would guarantee that buildTitle() was called after build().
But not now we can't set:
$page['entity']['#pre_render'][] = [$this, 'buildTitle'];
Because it won't have access to $page['#title'] and therefore it could not affect the page title.

So we set:
$page['#pre_render'][] = [$this, 'buildTitle'];
But then the Entity View builder build() method has not be called yet.

So hence we have to build the entity ourselves.

#46 had only coding standards voilations, unused use statments, fixing

Failing tests are not showing up at https://www.drupal.org/pift-ci-job/688355…

Here they are, extracted from the log:

00:04:26.064 Drupal\comment\Tests\CommentInterfaceTest                    420 passes   2 fails                            
00:04:58.596 Drupal\comment\Tests\CommentPagerTest                        362 passes   8 fails                            

I debugged \Drupal\comment\Tests\CommentInterfaceTest::testCommentInterface() some. The root cause is not what I thought it first was: a render cache item not being invalidated when it should. Because the entity_view:node:1:full:[languages:language_interface]=… render cache item does have the node:1 cache tag, and that is being invalidated when posting a comment on node 1.

So the problem must be related to how this patch changes the rendering of full node pages and that somehow affects the rendering of the "comment" field.

@Wim Leers yeah manually tested the problem here
CommentInterfaceTest::testCommentInterface()

// Confirm a new comment is posted to the correct page.
    $this->setCommentsPerPage(2);
    $comment_new_page = $this->postComment($this->node, $this->randomMachineName(), $this->randomMachineName(), TRUE);
    $this->assertTrue($this->commentExists($comment_new_page), 'Page one exists. %s');

When I post the 3rd comment on the node it does put the correct parameters in the query string to show the me the new page of comments but it still renders the first 2 comments.

UPDATE: Yes it looks like the problem is with \Drupal\Core\Entity\Controller\EntityViewController::buildTitle
and the fact it also builds the entity.
So \Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter::viewElements is called twice. The first time it loads the correct comment and the 2nd time it loads the incorrect comments, the first 2 on the node.

Regarding my comment in #54 this patch doesn't build the entity in \Drupal\Core\Entity\Controller\EntityViewController::buildTitle() but rather just builds the field with the options from the display.

This fix the 2 test mentioned in #52

Also posted this potential bug for DrupalCI #2884084: Test failures not showing up on test results page

So QuickEdit related tests failed because in quickedit_preprocess_field()

if ($element['#view_mode'] === '_custom') {
  return;
}

Because in #55 in \Drupal\Core\Entity\Controller\EntityViewController::buildTitle() it was using \Drupal\Core\Entity\EntityViewBuilderInterface::viewField to build the field the view mode was '_custom'.
So we just have to set the view mode because we know it at that point.

This should fix that test failures in #55. Hoping it doesn't cause any more!

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -61,16 +74,26 @@ public static function create(ContainerInterface $container) {
+      if ($component = $display->getComponent($label_field)) {
+        $field_build = $this->entityManager
+          ->getViewBuilder($entity->getEntityTypeId())
+          ->viewField($entity->{$label_field}, $component);

I'm wondering whether someone profiled this change. In views we did a LOT of changes to avoid calling viewField, given its really costly for rendering a single field. It could be to ensure in the ::view function that the label will actually be rendered, so you can always pick it out.

It could be better to ensure in the ::view function that the label will actually be rendered, so you can always pick it out.

I added that underlined + strong text. I think that's what you meant to say? I agree with that concern — I also raised it at the beginning of my comment in #43, where I asked:

Is it because we shifted things down a level (under the 'entity' key), and hence the things that buildTitle() was expecting to already been done, have actually not yet been done at this time, and hence you need to do it manually here? (Hence doing much of the work multiple times?)

However, I'm not concerned about the performance impact here, because the result is cached by Dynamic Page Cache anyway, so subsequent visits to the page won't run that code. I'm only concerned because the increase in complexity is massive:

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -61,16 +74,26 @@ public static function create(ContainerInterface $container) {
-      if (isset($page[$label_field])) {
-        $page['#title'] = $this->renderer->render($page[$label_field]);
+      $view_mode = isset($entity_output['#view_mode']) ? $entity_output['#view_mode'] : 'full';
+      $displays = EntityViewDisplay::collectRenderDisplays([$entity], $view_mode);
+      /** @var \Drupal\Core\Entity\Entity\EntityViewDisplay $display */
+      $display = array_values($displays)[0];
+      if ($component = $display->getComponent($label_field)) {
+        $field_build = $this->entityManager
+          ->getViewBuilder($entity->getEntityTypeId())
+          ->viewField($entity->{$label_field}, $component);
+        $field_build['#view_mode'] = $view_mode;
+        $page['#title'] = $this->renderer->render($field_build);

I stepped through this with a debugger, and I'm now pretty sure that my best-guess reasoning in #43 is actually correct.

Note that I am the one who moved shifted things down a level in #26, from $page = $view_builder->view($entity, 'full') to $page['entity'] = $view_builder->view($entity, 'full').
I did so to fix most [entity_type]CacheTagsTest tests. (Compare the failing tests in #25 vs #26.)
And given the additional insight I gained in #46, this is AFAICT a necessity. The old code was effectively polluting the render cache for nodes (see NodeViewControll::view(), which we are now bringing to all entity types).
This change is the solution.

I definitely would like to see this get a simpler solution though. After some tinkering, I think that's actually possible: what if we ensure that before buildTitle() is called, $page['entity'] has its #pre_render callbacks executed, so that $page['entity']['title'] exists, so that buildTitle() can continue to work as it does in HEAD?
EDIT: re-reading this, I think this is exactly what @dawehner was suggesting? :)

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -91,13 +129,53 @@ public function buildTitle(array $page) {
   +        $page['#attached']['html_head_link'][] = [
   +          [
   +            'rel' => $rel,
   +            'href' => $url->toString(),
   +          ],
   +          TRUE,
   

   I was always under the impression that canonical rel links should use the unaliased version of the url as it is the canonical source of the content?

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -91,13 +129,53 @@ public function buildTitle(array $page) {
   +        // Set the non-aliased canonical path as a default shortlink.
   +        $page['#attached']['html_head_link'][] = [
   +          [
   +            'rel' => 'shortlink',
   +            'href' => $url->setOption('alias', TRUE)->toString(),
   +          ],
   +          TRUE,
   +        ];
   

   Comment says non-aliased, but we are setting the alias option to TRUE?

3. +++ b/core/modules/taxonomy/taxonomy.module
   @@ -96,37 +95,6 @@ function taxonomy_term_uri($term) {
   -function taxonomy_page_attachments_alter(array &$page) {
   

   Awesome, this is causing problems with the Metatag module by adding duplicate canonical rel links.

   Question is, will this new approach play nice with the module? Seems like the existing approach for Node works fine.
   EDIT: Answer is, yes! metatag_entity_view_alter removes core's tags if metatag is adding them itself. Since taxonomy is doing it's own thing at the moment, it doesn't catch them.

Going to take a stab at fixing the tests.

Fixes some PHPCS and adds a check to $page['entity']['#pre_render']

Woops, wasn't meant to be a patch file.

Back to needs work to fix the test-failures, we should also add new tests for this functionality (or remove the needs tests tag).

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -35,10 +43,14 @@ class EntityViewController implements ContainerInjectionInterface {
   +   *   The current user. For backwards compatibility this is optional, however
   +   *   this will be removed before Drupal 9.0.0.
   

   This comment doesn't make sense, the parameter won't be removed, the optionality (is this a word?) of it will be removed. We should clarify this comment.

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -52,6 +64,32 @@ public static function create(ContainerInterface $container) {
   +    $this->controllerResolver = \Drupal::service('controller_resolver');
   

   Should we also make this an optional constructor argument? At minimum we should declare the controllerResolver as a protected variable.

3. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -183,10 +183,16 @@ public function toUrl($rel = 'canonical', array $options = []) {
   +        // Unsaved revisions cannot be linked to.
   

   This comment is useless as it is, it duplicates the message in the exception, we should either document why this is - or remove the entire thing.

4. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -183,10 +183,16 @@ public function toUrl($rel = 'canonical', array $options = []) {
   +        throw new EntityMalformedException(sprintf('The "%s" entity cannot have a revision URI as it is an unsaved revision yet.', $this->getEntityTypeId()));
   

   /s/yet//

#2791571: Automatically supply contextual links for entities just landed, would be great to now get this one to completion too :)

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -35,10 +43,14 @@ class EntityViewController implements ContainerInjectionInterface {
   +  public function __construct(EntityManagerInterface $entity_manager, RendererInterface $renderer, AccountInterface $current_user = NULL) {
   ...
   +    $this->currentUser = $current_user ?: \Drupal::currentUser();
   
   @@ -91,13 +131,53 @@ public function buildTitle(array $page) {
   +      if ($this->currentUser->isAuthenticated() || $url->access($this->currentUser)) {
   

   I'd better move this to protected method

2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -183,10 +183,16 @@ public function toUrl($rel = 'canonical', array $options = []) {
   +      elseif ($this->isNewRevision()) {
   +        // Unsaved revisions cannot be linked to.
   +        throw new EntityMalformedException(sprintf('The "%s" entity cannot have a revision URI as it is an unsaved revision yet.', $this->getEntityTypeId()));
   

   new exception should be documented & probably better to not throw the exception

Reroll against 8.7.x with a few minor changes:

#67.1 Updated the __construct comment to indicate the parameters will be required in Drupal 9.0.0.

#67.2 Moved the declaration to the __construct method.

#67.3 Added Wim's reasoning in #43 to the comment.

#67.4 Fixed.

#70.1 Not sure what part you would move? This is a cut/paste operation from the old NodeViewController.

#70.2 Not sure how to fix, but I'm open for suggestions.

#73.1 Fixed the test failures in ContentTranslation by adding the new user.role.anonymous cache context to the assertions

The failure in ModerationFormTest is due to the fact that hook_entity_view is now called twice for each entity rendering. This results in new element names being generated, eg. edit-new-state becomes edit-new-state--2. Not sure how to fix this.

Also the test coverage needs to be extended for different entity types.

Possibly controversial counter-proposal in #2922570: The node view page triggers a lot of expensive access checks for relationship links ;)

#2922570: The node view page triggers a lot of expensive access checks for relationship links is fixed.

#2873648: With many languages, content_translation_page_attachments adds too many alternate links to the response headers crashing varnish (503) is another critical bug caused by these that is still open.

Marking this as won't fix to avoid even more damage.