Regression: page cache tags broken, also: some routes incorrectly use _controller -> No page object in template variables -> Fatal in seven_preprocess_page()

You need to take care of the following piece of code:

system_test.cache_tags_page:
  path: '/system-test/cache_tags_page'
  defaults:
    _controller: '\Drupal\system_test\Controller\SystemTestController::system_test_cache_tags_page'
  requirements:
    _access: 'TRUE'

in system_test.routing.yml.

Awesome, thanks for fixing this.

I don't really think we wan't to add runtime code to template_preprocess_page() just to produce a test fail, though. I will look into this now and see if I can come up with something else.

So "now" didn't really happen because I fell asleep :-)
Will look into this later today, though...

So I did actually work on this yesterday, just forgot to upload the patch :-)
Here it is. It's just a slightly less hacky version of achieving some test-coverage.
I really could not think of any cleaner way of sending information from the test request to the testing request than an exception.
Let's see.
I also reverted the change to PageCacheTest, as that didn't seem to have fixed the test failure.

Oh and I forgot to mention: I looked through all *.routing.yml files in core for whether they use _controller incorrectly and only found the aggregator one that is contained in the patch above.

@likin: I just saw that it didn't fix the fail with the bot, so I reverted it. Did not try it locally yet, but will.

Also renaming this, because I couldn't find it just now :-)

We figured out the current problem. Before the HtmlPage issue drupal_render(drupal_prepare_page()) have been executed on the original controller render array,
which allowed to let cache tags to bubble up to #type page, and so gets called by drupal_post_render_cache_tags_page_set(). See system_element_info()

With the HtmlPage render pipeline we create a HtmlFragment first, so the cache tags no longer bubble up.

Crell, msonnabaum suggested to use the CacheableInterface from https://drupal.org/files/issues/blockcache-2158003-6.patch to pass the cache tags from the HtmlFragment to its parent and so to the page cache.

There are really ugly parts here (converting things back and forth).

1. +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
   @@ -39,9 +40,18 @@ class HtmlFragment {
   +      'max_age' => REQUEST_TIME,
   

   Shouldn't this be max_age 0? It's not expire time but "how long it should live from now". (At least I think that's what max_age is supposed to mean.)

2. +++ b/core/modules/simpletest/simpletest.module
   @@ -877,3 +878,20 @@ function simpletest_phpunit_testcase_to_row($test_id, \SimpleXMLElement $testcas
   +function simpletest_preprocess_page(&$variables) {
   +  // If controllers accidentally use _controller instead of _content, but do not
   +  // return a response object or an HTML page object the site still renders
   +  // properly but using the page object in template_preprocess_page() will fail.
   +  // We therefore throw an exception on the test site in this case so that every
   +  // call to WebTestBase::drupalGet() automatically benefits from this coverage.
   +  if (drupal_valid_test_ua()) {
   +    $page_object = $variables['page']['#page'];
   +    if (!($page_object instanceof HtmlPage)) {
   +      throw new \Exception('The page object is not an instance of \Drupal\Core\Page\HtmlPage');
   +    }
   +  }
   +}
   

   Incidentally, I love putting this check here. +1!

The latest patch looks like it is converting cache keys to cache tags. That looks very fishy. Am I missing something?

They aren't the same thing. Tags are only used for invalidation. Keys are used to distinguish different entries in the cache for hit/miss purposes. It is crucial to keep this distinction in mind.

What moshe said.

THis is actually a security issue, because drupal_render_cache_by_query takes node_grants into account when generating a cache key, so you won't get a cache hit for two users with different node grants.

@likin
I really think we should better continue with the approach of #2167039-27: Regression: page cache tags broken, also: some routes incorrectly use _controller -> No page object in template variables -> Fatal in seven_preprocess_page()
This won't introduce any security issue or something else.

#27looks quite sensible to me.

Figured out that cache tags are indeed multi-dim so you cannot just use implode('--').
I went with serialize/unserialize as I wasn't sure about a better alternative.

Let's see.

This should fix the remaining test failures.

47: controller-cache-2167039.patch queued for re-testing.

(Coming to this because I'm working on #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly, which was impossible to work on, the patch here is the solution.)

This is actually a huge regression introduced by [#2170551]: this completely broke cache tags bubbling up to cache entries in the page cache, which was introduced in #2094241: Cache tag the page cache. However, the test coverage added in #2094241 clearly was too superficial, or at least too artificial: if it would have a test case for e.g. the full node page and then checked that page cache entry's cache tags, this regression could've been avoided.

In this reroll:

• Adding test coverage to prevent such regressions i/t future.
• Removed drupal_post_render_cache_tags_page_set(), since that's being bypassed completely now: cache tags are passed to the page cache handling via Symfony response headers set in \Drupal\Core\EventSubscriber\HtmlViewSubscriber::onHtmlPage(), which are then retrieved by drupal_cache_tags_page_get().
• DefaultHtmlPageRenderer::render() was setting both $cache_info['tags'] and $cache_info['#tags']; the latter was ignored, so removed it.
• DefaultHtmlPageRenderer::render() was first getting the cache tags from the "page" HtmlFragment's cache info (as set on $page_content), but then it was also collecting the cache tags from $page_array after that was built from $page_content (confusing, right?!). AFAICT that's doing the same job twice.
• DefaultHtmlPageRenderer::render() wanted to make sure to pass on any caching info from the 'page' HtmlFragment to the HtmlPage that's being built by it. However, that just results in a bunch of ugly code, that might set expectations (i.e. "surely this cache bin and this max age and cache key will be used!"), even though it's completely ignored by the logic responsible for actually filling the page cache (drupal_page_set_cache()). By only passing the caching info that's actually used, I was able to simplify the code, and reduce the diff in that function to a single line being changed.

Thank you for helping on this issue!

The failure was so simple, that I just went and fixed it. Yes, simpletests have issues with logged in users in the UI vs. run-tests.sh
#2176101: drupalCreateNode uses different UID depending on UI vs. run-tests.sh is a follow up for that.

DefaultHtmlPageRenderer::render() wanted to make sure to pass on any caching info from the 'page' HtmlFragment to the HtmlPage that's being built by it. However, that just results in a bunch of ugly code, that might set expectations (i.e. "surely this cache bin and this max age and cache key will be used!"), even though it's completely ignored by the logic responsible for actually filling the page cache (drupal_page_set_cache()). By only passing the caching info that's actually used, I was able to simplify the code, and reduce the diff in that function to a single line being changed.

I am not sure whether this is a good assumption. If the CacheInterface has this information we should leverage it. Maybe block cache could use it or some fancy implementation one day. Let's discuss that in a follow up?

Haha, lovely! I was frustrated/puzzled on Friday night, I had no idea why it passed locally but failed on qa.d.o. Thanks a lot for fixing it! :)

I agree that we should pass all relevant cache metadata. But page caching is the top-level operation, which is governed by CMI settings + code, not by page-level metadata: cache keys depend on URL + content type, the cache bin is always the same and max age is governed by a CMI setting. Cache tags are passed. The patch I proposed is therefor the simplest patch possible.
So if you're saying we should figure out how to deal with the additional CacheableInterface metadata, then I agree :)

What's left to be done here? I think somebody that's not dawehner or I could mark this RTBC? tstoeckler perhaps? :)

Just to confirm that patch in #53, tested manually in simpletest.me, solves the issue I reported in #2177291: WSOD when trying to access configuration translation tab.

1. +++ b/core/lib/Drupal/Core/CacheableInterface.php
   @@ -0,0 +1,45 @@
   + * Contains \Drupal\Core\CacheableInterface
   

   Can we move this into the subdirectory of whichever core component it relates to?

   It really concerns me that \Drupal\Core itself turns more and more into a dumping ground for arbitrary classes/interfaces, and is deemed to be a "thing" in the first place. It was a big mistake that DrupalKernel (the first of this kind) was originally placed in there — even that belongs to a HttpKernel component.

2. +++ b/core/lib/Drupal/Core/CacheableInterface.php
   @@ -0,0 +1,45 @@
   +  public function cacheKeys();
   ...
   +  public function cacheTags();
   ...
   +  public function cacheBin();
   ...
   +  public function cacheMaxAge();
   

   It looks like all of these should be prefixed with "get"?

3. +++ b/core/lib/Drupal/Core/Controller/HtmlControllerBase.php
   @@ -84,4 +86,11 @@ protected function createHtmlFragment($page_content, Request $request) {
   +  protected function drupal_render_collect_cache_tags($element, $tags = array()) {
   

   Let's use methodCase and no drupal_ prefix?

4. +++ b/core/modules/simpletest/simpletest.module
   @@ -877,3 +878,20 @@ function simpletest_phpunit_testcase_to_row($test_id, \SimpleXMLElement $testcas
   +function simpletest_preprocess_page(&$variables) {
   +  // If controllers accidentally use _controller instead of _content, but do not
   +  // return a response object or an HTML page object the site still renders
   +  // properly but using the page object in template_preprocess_page() will fail.
   +  // We therefore throw an exception on the test site in this case so that every
   +  // call to WebTestBase::drupalGet() automatically benefits from this coverage.
   +  if (drupal_valid_test_ua()) {
   +    $page_object = $variables['page']['#page'];
   +    if (!($page_object instanceof HtmlPage)) {
   +      throw new \Exception('The page object is not an instance of \Drupal\Core\Page\HtmlPage');
   +    }
   +  }
   +}
   

   I'm afraid I do not understand why this change to simpletest is needed and what the test runner has to do with this issue?

   Simpletest module is not enabled in test child sites, so this code is not executed in tests in the first place. Only Simpletest module's own tests are enabling Simpletest module in tests (to test the test runner).

Lovely a review, thank you!

Can we move this into the subdirectory of whichever core component it relates to?

It really concerns me that \Drupal\Core itself turns more and more into a dumping ground for arbitrary classes/interfaces, and is deemed to be a "thing" in the first place. It was a big mistake that DrupalKernel (the first of this kind) was originally placed in there — even that belongs to a HttpKernel component.

I totally agree with you, so I went ahead and moved it to the cache namespace. I also agree with the DrupalKernel namespace issue, let's add an issue: #2177701: Move the DrupalKernel to a HttpKernel Component instead Drupal\Core

Let's use methodCase and no drupal_ prefix?

Camelcased it, though kept the drupal prefix, to make it as clear as possible what is going on. For example render() instead of drupalRender would be kind of confusing.

I'm afraid I do not understand why this change to simpletest is needed and what the test runner has to do with this issue?

This never worked ... and was kinda just there for debugging reasons. Let's just remove it again.

#56.2: regarding the "get" prefixes: I asked the same question, but msonnabaum had good reasons to not have the "get" prefixes. I'll ask him to chime in.

It looks like all of these should be prefixed with "get"?

Here's my reasoning for not using the "get" prefix.

The "get" prefix is used to communicate to the reader that the class is providing a public getter for it's internal state. Usually, but not always accompanied by a setter, the implication is that it maps to an instance property (whether or not that's actually true).

In this case, cacheKeys, cacheTags, etc, are part of a separate interface that implies no knowledge of the object's internal state. It's possible that those methods may simply return an instance variable, but that is an implementation detail that is not implied by the interface. It also makes no sense for these methods to ever have corresponding setters.

The get prefix here doesn't communicate anything useful to the reader. Is there anything ambiguous about what is returned from $object->cacheKeys()? I think it's pretty safe to assume that you're getting cache keys back.

Can we move this into the subdirectory of whichever core component it relates to?

It really concerns me that \Drupal\Core itself turns more and more into a dumping ground for arbitrary classes/interfaces, and is deemed to be a "thing" in the first place. It was a big mistake that DrupalKernel (the first of this kind) was originally placed in there — even that belongs to a HttpKernel component.

I disagree this is a bad thing at all. Namespaces aren't just a way to segregate code, they should communicate something. It is no clearer to have \Drupal\Core\Cache\CacheableInterface vs \Drupal\Core\CacheableInterface. On the other hand, DatabaseBackend clearly needs the Cache namespace to clarify it's context. It's a very common convention to only put classes/interfaces within a namespace when it clarifies the intent, not just as a grouping mechanism.

I don't want to start a bikeshed about the interface location, so I'm willing to concede here if others feel very strongly about.

However, I would ask that the method names be reverted.

Ah-ha! :) Thanks for the explanation, @msonnabaum.

That made sense. Although — after reading it, my next best thought was: Because it's a generic interface, isn't the fact that it does not care how about the actual class internals actually an argument in the opposite direction? — For example, cache items are still anonymous objects today, but if they were based on a dedicated CacheItem class, wouldn't that implement this interface, too? Wouldn't we have getters and setters at that point? :)

In general, I always connected the get/set/is method prefixes primarily with simple data/state management operations — as opposed to methods without such prefix, which I always interpreted to "do" something (more complex). E.g., all of the Entity::id() & Co methods are a misnomer from my perspective. But it's perfectly possible that my interpretation is wrong.

In any case, my original remark was meant as a minor review issue only; where applicable, we can easily rename method names later on, me thinks. No reason to hold up this patch; either way works for me.

That's an interesting example. I don't think that a cache item would implement this interface, but there would definitely be some naming overlap there. The difference would be that this interface is for things that can be cached using our cache api, providing the methods to create the cache section of a render array. A cache entry would implement an interface that documented it's identity as a value object.

I introduced a big flaw in #50. That flaw slipped through the cracks of the test coverage, because we only have cache tags for "content region" content so far. If we want to improve the page cache by removing the "content" cache tag (see #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly), then we must ensure that cache tags from all regions are picked up, not just the one in the "content region" of course.

I said:

DefaultHtmlPageRenderer::render() was first getting the cache tags from the "page" HtmlFragment's cache info (as set on $page_content), but then it was also collecting the cache tags from $page_array after that was built from $page_content (confusing, right?!). AFAICT that's doing the same job twice.

That is not doing the same job twice. It certainly looks so, because the current code is in a kind of transitional phase and therefor extremely misleading. drupal_prepare_page() invokes hook_page_build(), which adds the other regions to the page array… and therefor the changes I made were incorrect.

Adding test coverage is more onerous here than in #2179083: Rendered menus (e.g. menu blocks) should set cache tags to inform the page cache, which causes cache tags to be set on all menus, including the default "footer" menu, which would make it trivial to add test coverage. In fact, I've written the necessary test coverage in that issue (see \Drupal\system\Tests\Cache\PageCacheIntegrationTest in that patch). Since this patch blocks that patch, let us please get this in ASAP so we can continue there.

The "get" prefix is used to communicate to the reader that the class is providing a public getter for it's internal state. Usually, but not always accompanied by a setter, the implication is that it maps to an instance property (whether or not that's actually true).

You invented that, didn't you?

Really, it's a convention. The usual convention in PHP is to have a get prefix when you retrieve something from an object, regardless of how this is internally implemented. Other languages have different conventions (some languages have a set prefix but omit the get one, etc.).

Let's just stick to the convention.

Passes fine locally.

62: htmlpage-2167039-62.patch queued for re-testing.

Code looks fine. Its kinda off how we convert the fragment into a render array but thats not invented.

+++ b/core/lib/Drupal/Core/Controller/HtmlControllerBase.php
@@ -84,4 +86,18 @@ protected function createHtmlFragment($page_content, Request $request) {
+  protected function drupalRender(&$elements, $is_recursive_call = FALSE) {

What is this wrapping about?

On the whole this looks like a good approach. We're just passing along more cache data with the data objects in a structured fashion. A few comments:

1. +++ b/core/lib/Drupal/Core/Controller/HtmlControllerBase.php
   @@ -84,4 +86,18 @@ protected function createHtmlFragment($page_content, Request $request) {
   +  /**
   +   * Wraps drupal_render().
   +   */
   +  protected function drupalRender(&$elements, $is_recursive_call = FALSE) {
   +    return drupal_render($elements, $is_recursive_call);
   +  }
   +
   +  /**
   +   * Wraps drupal_render_collect_cache_tags()
   +   */
   +  protected function drupalRenderCollectCacheTags($element, $tags = array()) {
   +    return drupal_render_collect_cache_tags($element, $tags);
   +  }
   +
   

   These don't really have anything to do with HtmlControllers. They belong in their own service(s). Especially with the weird $is_recursive_call handling that drupal_render() requires. Separate that out to somewhere we can clean it up post-8.0 if needs be.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php
   @@ -65,6 +65,18 @@ public function onHtmlPage(GetResponseForControllerResultEvent $event) {
   +      if ($bin = $page->getCacheBin()) {
   +        $response->headers->set('cache_bin', $bin);
   +      }
   

   Putting the cache bin in the response seems quite weird to me, but I trust that Mark has some reason why we're doing that.

3. +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
   @@ -123,4 +140,39 @@ public function getTitle() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCacheKeys() {
   +    return $this->cache['keys'];
   +  }
   

   If this gets rerolled again, can we leave a @todo in here somewhere about turning this code into a trait? It's a perfect use case for it.

Coming from #2181427: Configuration translation only works with Stark. Trying to address #70.1 however not very sure where those 2 methods fit. Here is the options:

1. Can be part of DefaultHtmlPageRenderer.php
2. Can have a Page.php/default.php under Drupal\Core\Render and expose as service?

since #70.3 is nice to have, we can move forward after #70.1 fixed.

P.S As this is causing WSOD it most of non-default theme, getting lots of duplicate issues. Btw, any priority higher than 'critical'?

#69: the wrapping is to prevent crazy problems when writing unit tests: without the wrapping, each unit test class would have to define its own version of that procedural function, but there can only exist one. Chaos ensues. Therefor AFAICT dawehner is to prevent such chaos in the future.

Lots of duplicates, and blocking performance work too. Let us please get this in ASAP.

Really, it's a convention. The usual convention in PHP is to have a get prefix when you retrieve something from an object, regardless of how this is internally implemented. Other languages have different conventions (some languages have a set prefix but omit the get one, etc.).

+1

These don't really have anything to do with HtmlControllers. They belong in their own service(s). Especially with the weird $is_recursive_call handling that drupal_render() requires. Separate that out to somewhere we can clean it up post-8.0 if needs be.

Let's keep the signature 1to1 to just point out that it is really a wrapping. In general wrapping external functions is a tool to fix all kind of issues in unit tests.

Putting the cache bin in the response seems quite weird to me, but I trust that Mark has some reason why we're doing that.

We certainly should not use any kind of knowledge about the page caching here, maybe there are usecases we don't know yet.

If this gets rerolled again, can we leave a @todo in here somewhere about turning this code into a trait? It's a perfect use case for it.

Just added them.

Yeah let's speed up getting this in. None of the config translation tabs/paths/local tasks, etc. work currently except if you are using Stark (which was the reason for no prior test fails). That is sort of a big deal :D Not surprising about the many duplicates.

I don't want to use the functions directly. I'm saying that those methods should be on injected object(s). If we can't do that here for speed/throughput/unbreak things reasons, we should at least add @todos to note that as future refactoring.

Created follow up #2182149: Move drupalRender() and drupalRenderCollectCacheTags() out of HtmlControllerBase and added @todo

Adding tags...

Ready to get in then? :)

In my opinion: yes. We can nitpick this more, but since it blocks other work, I think it should get in ASAP. No matter what one dislikes about it, this patch definitely improves current D8 HEAD.
I can't RTBC it though: I worked on it.

My vested interest in this issue leads as far as to fix the controller/content callbacks :) Since all other issues were closed as duplicates on this and we need to fix that ASAP, let's get this in and argue about specifics in followups.

Sorry to downgrade this again, I didn't make the rules, but: 1. violates the docs gate. (Elsewhere as well.)

1. +++ b/core/lib/Drupal/Core/Cache/CacheableInterface.php
   @@ -0,0 +1,44 @@
   +  /**
   +   * @return array
   +   *   An array of strings or cache constants, used to generate a cached id.
   +   */
   +  public function getCacheKeys();
   

   There should be a one-line description here.

   Also id -> ID

2. +++ b/core/lib/Drupal/Core/Cache/CacheableInterface.php
   @@ -0,0 +1,44 @@
   + * Defines an interface for things which are potentially cacheable.
   

   Why "potentially"? Either they are cacheable or they aren't. And if they implement this interface I'd say they are. Is that incorrect?

3. +++ b/core/lib/Drupal/Core/Cache/CacheableInterface.php
   @@ -0,0 +1,44 @@
   +   *   Returns TRUE if the object is cacheable, otherwise FALSE.
   

   "FALSE otherwise" instead "otherwise FALSE".

#81: those are precisely the details I meant when I said "No matter what one dislikes about it […]" :)

1. Fixed.
2. Proof by counter example: most blocks are cacheable, but not all blocks are. Therefor: "potentially cacheable". That's also why there is the isCacheable() method.
3. Fixed.

All good now back to RTBC.

Great work! This seems catch-ish, so assigning to him.

No complaints. Committed/pushed to 8.x, thanks!

Could use a change notice for the new interface.

Thanks!

This has now unblocked two important cache tags issues:

1. #2179083: Rendered menus (e.g. menu blocks) should set cache tags to inform the page cache
2. #2183017: Views doesn't bubble up rendered entities' cache tags

msonnabaum is writing the change notice :)

Is it appropriate to rediscuss the design of CacheableInterface here, or do we need a follow up for that too?

#88: let us please do that in a follow-up.

Here's a change notice for the interface: https://drupal.org/node/2184553

Never written one before, so I likely did something wrong.

#90: hehe :) You didn't reference this issue, and you kept it as a draft (which was added somewhere in the last few days), hence it didn't show up at https://drupal.org/list-changes/drupal.
Then, you should also update the metadata of this issue, like I'm doing here now.

But those are the easy parts :) Thanks! :)