Problem/Motivation

#2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!)

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Status: Active » Needs review
StatusFileSize
new1013 bytes

This fixes it, but seems *extremely* heavy-handed.

dsnopek’s picture

So, I don't know smart cache at all, but could you do something like this:

Cache::invalidateTags(['page_manager_page:' . $entity->id()]);

... except with (a) the right API and what-not (since you're not being passed the entity object) and (b) making sure we actually have that cache tag set somewhere (which I don't think we do right now).

tim.plunkett’s picture

StatusFileSize
new11.43 KB

As far as I can see, this should fix it. But it doesn't...

tim.plunkett’s picture

StatusFileSize
new11.45 KB
new2.69 KB

F*&%ing priorities... This should pass.

wim leers’s picture

+++ b/src/EventSubscriber/RouteNameResponseSubscriber.php
@@ -0,0 +1,61 @@
+      $cacheability_metadata->addCacheTags(['route_name:' . $this->routeMatch->getRouteName()]);

This is a very generic cache tag. I'd say it should be page_manager_route_name:$route_name or something like that, to prevent a conflict in case core ever does something like that.

Otherwise looks great :)

dsnopek’s picture

@Wim Leers: Tim and I discussed on IRC last night that this is something Views might need as well (Tim's going to do some research/testing today) so it very well might make sense for core to do this.

Although, would this really be a "conflict"? Isn't adding a cache tag that's already there just a no-op? If core later starts adding this same cache tag, then the subscriber could be removed and no other code would need to be change, which might actually be a good thing. :-)

tim.plunkett’s picture

Status: Needs review » Fixed

I think @dsnopek has a good point, in that ideally this would be harmless to squat on the namespace. But it's Not The Right Way™, so I'll commit this with a namespaced tag to get tests passing, and then follow up on a core issue.

  • tim.plunkett committed 4dba56c on 8.x-1.x
    Issue #2565887 by tim.plunkett: Fix test failures stemming from Dynamic...

The last submitted patch, 4: 2565887-smartcache-4.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 5: 2565887-smartcache-5.patch, failed testing.

The last submitted patch, 4: 2565887-smartcache-4.patch, failed testing.

The last submitted patch, 5: 2565887-smartcache-5.patch, failed testing.

wim leers’s picture

Although, would this really be a "conflict"? Isn't adding a cache tag that's already there just a no-op? If core later starts adding this same cache tag, then the subscriber could be removed and no other code would need to be change, which might actually be a good thing. :-)

Hah! Genius! :)

tim.plunkett’s picture

Status: Needs work » Fixed

:)

There are actually other bugs still, just missing test coverage. Will open a new issue shortly.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.