/node/add is lacking cacheability metadata, causes problems when cached by Dynamic Page Cache and "Use admin theme when editing or creating content" is turned off

Can this be related to #550254: Menu links are sometimes not properly re-parented? If I remember correctly (https://www.drupal.org/node/550254#comment-3851154) there was a link between that issue and the "You have not created any content types yet" message.

Working on a test

The fix is probably all about adding the right cache tags.

Removed the GIF, because I don't want mobile users to download 2MB.

The fix would be to change \Drupal\Core\Entity\EntityListBuilder::render to also include the entity list tag.

Confirmed, some cache/menu routing is not cleared/refreshed at the right moment I presume

Same happens when deleting a content type, you'll still see the content type when clicking 'Add content' (note, best to add 2 first, clear cache, then delete), so we should probably write a test for that as well.

Added the task as novice task.

Oh well, actually this is the wrong fix, but probably also a cause for potential many critical bugs

OK, I'll try to make a patch.

This adds the cache tags.

@dawehner I'm not sure what you mean by "access cache contexts". I'm still learning about cache context and tags.

I'm not sure what you mean by "access cache contexts". I'm still learning about cache context and tags.

I've explained this to @marcvangend in IRC :)

New patch, adds the cachability metadata from the node type access object.

BTW the patch in #17 already came back green, so that would mean test coverage isn't 100% yet.

@YesCT asked me to post #18 here.

[2015-10-04 12:58:51] <WimLeers> marcvangend: you wondered about "access cache contexts"
[2015-10-04 12:58:58] <WimLeers> marcvangend: so in NodeController::addPage(), you see this:
[2015-10-04 12:58:59] <WimLeers> if ($this->entityManager()->getAccessControlHandler('node')->createAccess($type->id())) {
[2015-10-04 12:59:05] <marcvangend> WimLeers: yes
[2015-10-04 12:59:12] <WimLeers> i.e. we only show certain things IF the user has access
[2015-10-04 12:59:43] <WimLeers> marcvangend: so that means that page *varies* by certain cache contexts: perhaps by default, Drupal core grants access or not based on permissions
[2015-10-04 13:00:07] <WimLeers> marcvangend: but you could change the access control logic on your site to grant access based on group, specific user, role, whatever
[2015-10-04 13:00:12] <WimLeers> marcvangend: even phase of moon if you like :P
[2015-10-04 13:00:31] <marcvangend> WimLeers: yes - that's what cache context is all about if I understand correctly
[2015-10-04 13:00:44] <WimLeers> marcvangend: So, bottom line, we need to make sure that  the render array returned by NodeController::addPage() contains the cacheability metadata associated with the access check
[2015-10-04 13:01:10] <WimLeers> marcvangend: now, if you follow if ($this->entityManager()->getAccessControlHandler('node')->createAccess($type->id())) { to \Drupal\Core\Entity\EntityAccessControlHandlerInterface::createAccess(), you see that ::createAccess() actually accepts many more arguments
[2015-10-04 13:01:22] <WimLeers> marcvangend: by default it returns a bool, this was done to minimize disruption
[2015-10-04 13:01:41] <WimLeers> marcvangend: but in this case, we want to get an AccessResultInterface object, which contains cacheability metadata
[2015-10-04 13:01:51] <WimLeers> marcvangend: i.e. createAccess($entity_bundle = NULL, AccountInterface $account = NULL, array $context = array(), $return_as_object = FALSE) -> we want to pass TRUE for that last param
[2015-10-04 13:02:15] <WimLeers> marcvangend: so then we get a value object that describes whether access is granted or not AND the cacheability metadata of that access result
[2015-10-04 13:02:24] <WimLeers> marcvangend: so it also contains the appropriate cache contexts
[2015-10-04 13:03:44] <WimLeers> marcvangend: so you want to pass TRUE there, assign it to a variable ($access = …->createAccess(…, NULL, [], TRUE)), check if access is granted ($access->isAllowed()) and ALWAYS, even if access is not granted, store the cacheability metadata for the access result on the render array
[2015-10-04 13:03:58] <WimLeers> marcvangend: which you can do using Renderer::addCacheableDependency($build, $access)
[2015-10-04 13:04:23] <WimLeers> marcvangend: example at \Drupal\system_test\Controller\SystemTestController::permissionDependentContent
[2015-10-04 13:04:56] <marcvangend> WimLeers: OK, I'm going to let that sink in for a moment :-)
[2015-10-04 13:05:05] <WimLeers> marcvangend: if you want to read more about that: https://www.drupal.org/node/2337377
[2015-10-04 13:05:07] <Druplicon> https://www.drupal.org/node/2337377 => All access-checking logic must now return AccessResultInterface objects, allows for cacheability metadata => 0 comments, 5 IRC mentions
[2015-10-04 13:07:27] <WimLeers> marcvangend: I can hop on Skype if that helps :)
[2015-10-04 13:08:18] <marcvangend> WimLeers: Thanks. I'll start following the code and see if I can make it work now.
[2015-10-04 13:08:32] <WimLeers> marcvangend: ping me if you're stuck.

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -72,13 +72,22 @@
   +    $output = [
   

   We usually call this $build.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -72,13 +72,22 @@
   +      $access = $this->entityManager()->getAccessControlHandler('node')->createAccess($type->id(), NULL, [], TRUE);
   +      if ($access->isAllowed()) {
            $content[$type->id()] = $type;
          }
   +      $this->renderer->addCacheableDependency($output, $access);
   

   Perfect :)

BTW the patch in #17 already came back green, so that would mean test coverage isn't 100% yet.

Indeed, we are missing some assertCacheTag() and assertCacheContext() assertions.

OK, $build it is.

confirmed with @Wim Leers that no change record is needed (and filled out more in the issue summary)

This should test the presence of cache tags and context.

Really small nitpick. :)

Also, I was wondering if this happened with Standard profile and it doesn't. The difference is the field_ui module. If I enable field_ui module, this issue doesn't exist anymore. I am not sure what exactly field_ui does but dynamic_page_cache can't find the node/add in cache anymore. Without field_ui, it always returns a cached version.

Cross-post. Here is the combined patch from #24 and #25.

Thanks Hussain, that comma may be small, but very annoying when forgotten.

The root cause is missing cacheability metadata. This issue fixes that.

The reason this is only a problem with the Minimal install profile is that in that install profile, the /node/add route (node.add_page) is NOT marked as an admin route (_admin_route: TRUE), but in the Standard install profile it is.

Dynamic Page Cache does not cache responses for admin routes. Hence in Standard profile, there is no problem (/node/add is an admin route), but in the Minimal profile there is a problem (/node/add is not an admin route).

And finally, the reason Standard marks it as an admin route and Minimal does not: this line in standard.install:

  // Enable the admin theme.
  \Drupal::configFactory()->getEditable('node.settings')->set('use_admin_theme', TRUE)->save(TRUE);

Plus this in NodeAdminRouteSubscriber:

  protected function alterRoutes(RouteCollection $collection) {
    if ($this->configFactory->get('node.settings')->get('use_admin_theme')) {
      foreach ($collection->all() as $route) {
        if ($route->hasOption('_node_operation_route')) {
          $route->setOption('_admin_route', TRUE);
        }
      }
    }
  }

Correcting the title to reflect the actual cause.

@Wim Leers: I agree that the root cause for this issue is fixed; however, I am still confused and concerned that there might be something about the field_ui module we are not taking into account here. I had tested earlier and to make sure, I tested again.

1. Install a clean site with minimal profile.
2. Add two content types - foo and foo2.
3. Go to 'Add Content' page and see that there are no types listed (this is the issue here).
4. Enabled 'Field UI' module. Of course, cache is cleared and now those two types are shown.
5. Add a new content type - foo3.
6. Go to 'Add Content' page, and now we see all the three types listed.
7. Repeat, and now we see the new content type as well.
8. The 'Add Content' page works now, even in minimal profile (just with field_ui enabled).

In short, either field_ui does something to fix this OR the presence of at least one other content type fixes the caching. I can't verify if that is the case because once field_ui is enabled, I can't disable it.

fyi, did a git bisect
5e8523ecb95f1530e80ea2d9929135ef9b3f8733 is the first bad commit
#2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!)

Yep, that makes sense, Dynamic Page Cache exposed a long pre-existing problem: missing cacheability metadata on that route.

Yep, that makes sense, Dynamic Page Cache exposed a long pre-existing problem: missing cacheability metadata on that route.

In other words we should require #cache on every controller which returns a render array, to ensure what is going on.

@hussainweb
The reason why field_ui helps here is that field_ui rebuilds the router in field_ui_entity_bundle_create()
which causes the 'route_match' cache tag to be invalidated, see \Drupal\Core\EventSubscriber\CacheRouterRebuildSubscriber::onRouterFinished

This cache tag is part of every page, given that almost always something requires it.

@marcvangend thanks for the tests in #24

it is common to make a tests only patch, and a patch with the tests and the fix. and let the testbot run on both. That way, the reviews (and committers) can see the test is failing and in the correct way.

tinying up the issue summary

@Wim Leers
Well to be fair otherwise this bug would have not been exposed given how often anonymous users have access to /node/add

#2579887: EntityListBuilder requires cache tags is another issue like that

@dawehner: Thanks for the explanation. That makes sense.

Not blocking any of this, RTBC + 1 to the patch. Just some thoughts:

What I don't like about all of this is:

- Why do we conditionally make something an admin route?

I don't think node/add should ever be admin. I think the decision to use admin_theme should be a new property admin_theme as usually admin route denotes something else - I think. Though I might be wrong; Do we have a definition of what admin_route means on a route?

Why do we conditionally make something an admin route?

This was my concern also, I use the minimal profile because I don't want to spend time trawling through all the modules that I don't need for my project and deleting the content type bloat, I wouldn't expect any conditional stuff like this, especially not potentially security related stuff that could bite me at a later date.

#40 + #41: That's a completely separate problem, and a design decision that was made many years ago. Can you please discuss that in a separate issue, so that this one doesn't get derailed? Thanks.

I think the patch is good, however it likely would have been simpler to just add max-age=0 or no_cache: TRUE for the critical bug fix of this, then make it cacheable later.

Committed db0ece0 and pushed to 8.0.x. Thanks!