The node view page triggers a lot of expensive access checks for relationship links

#2406533: edit-form, delete-form etc. <link> tags added on /node/{node} are invalid according to W3C Validator added the original access check

Reading through #1970360: Entities should define URI templates and standard links it was never discussed that this might become a performance issue.

I am also wondering whether we could make this somehow an opt IN feature rather than an opt out feature.

AAMOF the access checks for many node routes, including the edit form, are performed anyway while generating local tasks.

I personally haven't seen those calls, but maybe I was blind, or render caching got applied to those blocks.

Does render caching get applied to the HEAD links too? I think it does now so this might be moot after all. I left a comment on the main issue which would help the specific case there though.

Does render caching get applied to the HEAD links too?

These links are generated on the controller level, which are only cached by the page level caching (page cache vs. dynamic page cache).

Fair point, let's hope the best as always.

#1970360: Entities should define URI templates and standard links added the links, #2406533: edit-form, delete-form etc. <link> tags added on /node/{node} are invalid according to W3C Validator added the access checking. Because it was causing W3C validation errors. The performance problems were even pointed out (incidentally by yours truly): #2406533-67: edit-form, delete-form etc. <link> tags added on /node/{node} are invalid according to W3C Validator. Ah well.

So here's a pretty aggressive proposal to resolve this. From the original issue that added this:

> We can do the same in HTML
elements. Hypermedia FTW, again. (What could we do with that information client-side? The mind boggles!)

Yeah, my mind is indeed still boggled about it. As in, I don't understand the use case, to be honest. I get it for REST/JSON-API, fair enough, as these are then standardized protocols that can be more or less understood by generic clients. (And also fair point that for a decoupled site that actively uses those endpoints instead of the HTML format, this won't actually help, I get that too). But some random HTML forms, I can't imagine what the benefit there is.

If someone has a use case, IMHO a contrib module relatively easy provide this as well for all entity types with some trivial checks on the route name. Like content_translation_page_attachments() already does for translations and taxonomy does it in a hook too: taxonomy_page_attachments_alter().

Beside this performance issue, there are also #2856823: delete-form and edit-form links in header for anonymous users and #2821635: edit-form, delete-form etc. <link> tags added on /taxonomy/term/{taxonomy_term} are invalid according to W3C Validator (duplicate) to add access checks to terms as well. And [#2782283 ] about making it more generic.

So I'd propose to just keep the canonical and shortlink as hardcoded special cases but do those for all entity types*. We might also want to move content_translation_page_attachments() into a default implementation, as everything it depends on is actually generic entity API concepts, content_translation is just about the backend UI. We would need access checks for the translations but not the current language IMHO, because we know that you can access it.

To be defined: BC? Opt-in? Opt-out? This first patch is just to show how it could look like.

I guess that would break on node preview, this should fix that. Although preview of an already saved entity would still add those, but we could unset them in the node preview controller if we care.

To me this seems like 'data structure' - so could go into a minor release without a bc layer. Would obviously be possible to do something with Settings for opt-in/opt-out though if we can think of something the change would actually break.

Had the wrong variable name, so that actually didn't do much yet. This should be better. Also fixed a bunch of tests, but adding url.site to more responses (had to make the URL absolute so we do need that cache context unfortunately, no way around it) might fail some additional ones.

Also fixed a bug in \Drupal\Tests\node\Functional\NodeViewTest::testLinkHeader(), it passed the alias = TRUE option to the wrong method. Was wondering about adding an alias to test that explicitly, but skipping that for now as it's also tested in some other places like \Drupal\Tests\path\Functional\PathAliasTest::testNodeAlias().

Some more test fixes around cache contexts. We could still split up the part that's making it the default from simplifying node/term, would require fewer test changes.

Reroll for 9.1.x, conflict on setUp() return type changes.

+++ b/core/modules/node/src/Controller/NodeViewController.php
@@ -63,51 +63,7 @@ public static function create(ContainerInterface $container) {
-
-    return $build;
+    return parent::view($node, $view_mode);
   }

This isn't passing $langcode. Also do we need the method at all now?

Otherwise looks great. I'd prefer to make this change than #2856823: delete-form and edit-form links in header for anonymous users.

Thanks for the review.

> This isn't passing $langcode. Also do we need the method at all now?

The parent method doesn't have a $langcode parameter, that was always bogus or at least for a very long time.

Not sure if I remember why I left the method, would have to look it up. BC maybe? I think there was some reason.

Fixed the minor CS failure.
removed the unused use statement.

Can't see why we'd need bc for the method, even if something passed $langcode or extended the controller it shouldn't make a difference.

Since my question was answered and the coding standards issue has been fixed, moving this to RTBC.

Adding issue credit from #2856823: delete-form and edit-form links in header for anonymous users which I'm about to mark as duplicate.

This needs a re-roll unfortunately.

Also marking #2454915: Entity link annotations in HTML head are not valid HTML as duplicate and adding credit from there.

Added reroll of patch #31.

I think we need a release note to document that node html output is changing. Also this should have a change record which details which link relationships are being removed and how to add them back if they are being relied upon.

Fixing the tests.

Moving back to NW as the Change Record need to be created.

This all looks promising. Haven't super closely reviewed, yet, but a quick skim looks good.

Sorry for the noise, but I'm going to be running this on a 8.9.x site, and the latest patches don't apply there. So here's a composer-friendly reroll. ;)

Version change wasn't intentional...

Let's see if Version 9.2.x-dev sticks now.

Reroll against latest core changes.

+++ b/core/modules/node/src/Controller/NodeViewController.php
@@ -63,51 +63,7 @@ public static function create(ContainerInterface $container) {
+    return parent::view($node, $view_mode);

This method can be deleted entirely, as it just calls the parent? This was also noted in #29-32 but I don't see the point of keeping it.

The variable name is different between the methods, which is relevant for controller methods.

Oh I see. Worth a followup to deprecate that and unify it somehow?

Added a release note snippet and change notice: https://www.drupal.org/node/3216941

The code looks good to me, so back to RTBC.

great work on this one, I did some XHProfGUI profiling not long ago and confirm that we do need the performance enhancement. Has anyone done any profiling work on the latest patch yet? just wondering.

I recently used XHProfGUI and would recommend it but it did take some effort to set up. https://github.com/perftools/xhgui

It would be nice to know how much of a performance gain this patch will bring (approximately).

I can try, but there's likely not a simple answer for this, as it depends a lot on what kind of node access checks you have. If you have node grants, are using modules like group/og or have expensive access logic in custom hooks then the benefit is going to be much bigger than just with core.

There might also be fewer cache variations and cache redirects in some cases, which is not something you can quantify easily with profiling of single requests.

Ya for profiling access checks here make sure to use a non-administrative role as the admin user account bypasses all these checks so to test access check improvements you'd want to use a different role/account.

Yes, not just non-adminstrative but explicitly as anon user, because the old logic only checked access for anonymous users.

Did some testing. Disabled page and dynamic page cache to simulate uncached pages for anonymous users, created a very simple page node.

ab -n 100 -c 1 http://d8/node/2 without blackfire and xdebug extensions:
HEAD:
Requests per second:    39.40 [#/sec] (mean)
Time per request:       25.380 [ms] (mean)

Requests per second:    38.27 [#/sec] (mean)
Time per request:       26.132 [ms] (mean)

Patch:

Time per request:       24.392 [ms] (mean)
Time per request:       24.392 [ms] (mean, across all concurrent requests)

Requests per second:    41.22 [#/sec] (mean)
Time per request:       24.258 [ms] (mean)

Requests per second:    41.33 [#/sec] (mean)
Time per request:       24.198 [ms] (mean)

Actually surprised to see a bigger improvement as I expected here. Comparing also with blackfire and seeing a considerable improvement there as well (as always, take with a grain of salt as blackfire tends to considerable increase the cost of function calls. Response time there is 50% higher than with ab). Url/Route access checking really is expensive, especially of entity routes. We need to load the route, do upcasting and all kinds of other things beside just checking access.

So yes, lets do this :)

Considering the performance gains and amount of issues that were closed as duplicate, I'd even say this could be set to major.

I also added a code snippet and references to the change record if someone wants to have those relationships back.

Thanks for working on this folks, will be great to see this solved once and for all.
Got a couple of questions.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -101,6 +101,32 @@ public function view(EntityInterface $_entity, $view_mode = 'full') {
   +      // Set the non-aliased canonical path as a default shortlink.
   ...
   +          'href' => $url->setOption('alias', TRUE)->toString(),
   

   Here we say 'non-aliased' but then we use set alias to TRUE? Wouldn't non-aliased use FALSE?

   I've searched the code in UrlGenerator and I don't see any mention of 'alias'

   I was expecting this to use 'path_processing'

2. +++ b/core/modules/comment/tests/src/Functional/CommentCacheTagsTest.php
   @@ -158,4 +159,11 @@ protected function getAdditionalCacheTagsForEntity(EntityInterface $entity) {
   +    return ['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme', 'user.permissions'];
   
   +++ b/core/modules/content_translation/tests/src/Functional/ContentTestTranslationUITest.php
   @@ -14,6 +14,11 @@ class ContentTestTranslationUITest extends ContentTranslationUITestBase {
   +  protected $defaultCacheContexts = ['languages:language_interface', 'theme', 'url.query_args:_wrapper_format', 'user.permissions', 'url.site'];
   

   nit: as arrays > 80 chars these should be wrapped

3. +++ b/core/modules/node/src/Controller/NodeViewController.php
   @@ -63,51 +63,7 @@ public static function create(ContainerInterface $container) {
   +    return parent::view($node, $view_mode);
   

   now this defers to the parent, is there any need to keep it?

4. +++ b/core/modules/node/tests/src/Functional/NodeViewTest.php
   @@ -25,50 +25,11 @@ public function testHtmlHeadLinks() {
   +    $element = $this->assertSession()->elementExists('css', 'link[rel="shortlink"]');
   +    $this->assertEquals($node->toUrl('canonical', ['alias' => TRUE])->setAbsolute()->toString(), $element->getAttribute('href'));
   

   If 'alias' => TRUE is indeed not supported, then this shows the test is too close to the implementation, we should instead be compiling the expected uri manually, e.g. 'node/' . $node->id()

5. +++ b/core/modules/node/tests/src/Functional/NodeViewTest.php
   @@ -79,8 +40,7 @@ public function testLinkHeader() {
   -      '<' . Html::escape($node->toUrl('canonical')->setAbsolute()->toString(), ['alias' => TRUE]) . '>; rel="shortlink"',
   

   hmm but we're using alias TRUE in HEAD

   I wonder if that's broken too 🤔

Thanks for the review.

1./5. The alias option is weird, but correctly use. Easy to verify, just look at an article that does have an alias and you can see that shortlink is node/N. It kinda means "this url is already an alias, do not look for an alias again". Super weird, but has been like this for as long as I can remember. The option is in \Drupal\path_alias\PathProcessor\AliasPathProcessor. Disabling path processing entirely is not the same thing and not what we want, that would also not add the language prefix then for example.

3. See #70/#71.

4. I get the point, but it's an absolute link, so we would also need to replicate adding the domain/path to it and so on. This method apparently didn't test shortlink before, but there is an existing test just below that for this. And all existing tests also used toUrl().

Leaving at needs work for #2.

Here is patch that addresses #78.2
I also changed protected $defaultCacheContexts = ['languages:language_interface', 'theme', 'url.query_args:_wrapper_format', 'user.permissions', 'url.site']; in UserTranslationUITest.php
and
protected $defaultCacheContexts = ['languages:language_interface', 'theme', 'url.query_args:_wrapper_format', 'user.permissions', 'url.site']; in TermTranslationUITest.php

All points in #78 answered or fixed, back to RTBC.

1./5. The alias option is weird, but correctly use. Easy to verify, just look at an article that does have an alias and you can see that shortlink is node/N. It kinda means "this url is already an alias, do not look for an alias again". Super weird, but has been like this for as long as I can remember. The option is in \Drupal\path_alias\PathProcessor\AliasPathProcessor. Disabling path processing entirely is not the same thing and not what we want, that would also not add the language prefix then for example.

Yeah I think this dates back about 12-15 years or so, must be barely used any more except in this issue. The 'alias' option used to mean "this is an alias, don't try to look up an alias for it again", in this issue it's being used purely to disable alias look-up. So I think we should open a follow-up to rename the option to something a bit more semantic - have done so here: #3218088: Rename the 'alias' option for URLs.

Nice to see how much this cleans things up too.

Committed bfe59b2 and pushed to 9.3.x. Thanks!

I will leave it up to others to decide if this is a release highlight or not.

I think it might be, adding the tag.

thanks @Berdir, I see the code now.

again this slide proves true http://larowlan.github.io/ds.core-til/#/9/2

glad to see this fixed

Too bad it missed 9.2.0

we need speed and are impatient so I'm rerolling backports

Reroll for D91x and also applies cleanly for HEAD of D92x (same patch for both)

#88 no longer applies to 9.2.x branch. Here's another backport for composer folks.

Sorry for the noise, but #89 only applies to 9.2.x branch. If you've got 9.2.4 official, it doesn't apply due to #3139404: Replace usages of AssertLegacyTrait::assertText, that is deprecated. So here's one that should apply to 9.2.4 itself...

Patch in #90 wasn't working for me on drupal 9.2.5, here the reroll for Drupal core 9.2.5

Patch #91 works for me on drupal 9.2.6. thx.

Hello,

I tested the Patch #91 on drupal 9.2.5 and it works.

Thank you !