(#4870) Remove deprecated CacheDecoratorInterface from AppPathManager

[ChadChandlerPS]

CacheDecoratorInterface is deprecated as of Drupal 10.2.0 and removed in Drupal 11.0.0 with no replacement (see
https://www.drupal.org/node/3398182).

Updated the now orphaned @inheritdoc docblocks to describe them directly.

Closes #4870

[ChadChandlerPS]

Testing Done on this PR

• Confirmed via grep that no reference to CacheDecoratorInterface remains anywhere in the codebase after the change.
• Ran the app_module module's PHPUnit unit test suite (AppPathManagerGetPathTest, AppPathManagerUpdatePathDataTest, PathProcessorAppModumeTest, etc.) via ddev exec vendor/bin/phpunit -c docroot/core --group app_module - 10 tests, 36 assertions, all passing
• Checked whether setCacheKey()/writeCache() (the two methods from the deprecated interface) are actually called anywhere (they're not, they're dead code). So dropping the interface doesn't change any runtime behavior. cacheClear() also isn't called anywhere as it's just declared on our own AppPathManagerInterface, not the deprecated one -- left untouched regardless.

[bryanpizzillo]

@ChadChandlerPS - what do you mean by the following:

Checked whether setCacheKey()/writeCache() (the two methods from the deprecated interface) are actually called anywhere (they're not, they're dead code).

[ChadChandlerPS]

While working this, I traced every caller of these caching-related methods across docroot/modules/custom and docroot/profiles/custom.

Finding

AppPathManager has a caching layer built around three methods: 1)setCacheKey($key), 2)writeCache(), and 3)cacheClear($source = NULL). None are called anywhere in the codebase:

$this->cacheKey is never set, so initPathData()'s cache-read branch never fires.

Every request that reaches this service just does a plain DB query via loadFromStorage() - no caching happens today despite the scafolding.

cacheClear() is declared on the module's own AppPathManagerInterface and looks like it should fire on path alias insert/update/delete, but nothing actually calls it.

This whole thing predates this ticket and removing the deprecated interface has no effect since these methods were already unreachable.

[ChadChandlerPS]

what do you mean by the following:

Checked whether setCacheKey()/writeCache() (the two methods from the deprecated interface) are actually called anywhere (they're not, they're dead code).

@bryanpizzillo To clarify - CacheDecoratorInterface (the deprecated one this PR removes) only declared two methods: setCacheKey() and writeCache(). Neither is called anywhere in the codebase, so removing the interface doesn't change any behavior ... it was already dead code.

After digging into this more one correction to my earlier note: I said cacheClear() was "the one that's actually used" and that's not right. I traced it further and it's never called either, it just happens to be declared on our own AppPathManagerInterface instead of the deprecated one. So all three caching methods on this class are dead, not two of three. We should probably wire the caching up for real or remove it (the dead scaffolding)

[bryanpizzillo]

Well I will agree that the CacheDecoratorInterface is dead. It was only even "used" in Drupal for the AliasManager and when Aliases became entities things all changed. Technically the interface was never used at all - nowhere in the code did something expect a CDI to be passed in. The only thing that used the methods defined by the CDI was PathSubscriber and that expected a AliasManagerInterface, not a CacheDecoratorInterface...

Now, here is the interesting thing, the PathSubscriber code loaded all aliases onKernelController and then wrote the cache onKernelTerminate. Subsequent requests would read from the cache. Technically they cached each individual route, where our code would store all.

  /**
   * Sets the cache key on the alias manager cache decorator.
   *
   * KernelEvents::CONTROLLER is used in order to be executed after routing.
   *
   * @param \Symfony\Component\HttpKernel\Event\FilterControllerEvent $event
   *   The Event to process.
   */
  public function onKernelController(FilterControllerEvent $event) {
    // Set the cache key on the alias manager cache decorator.
    if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {
      $this->aliasManager->setCacheKey(rtrim($this->currentPath->getPath($event->getRequest()), '/'));
    }
  }

  /**
   * Ensures system paths for the request get cached.
   */
  public function onKernelTerminate(PostResponseEvent $event) {
    $this->aliasManager->writeCache();
  }
  
  /**
   * Registers the methods in this class that should be listeners.
   *
   * @return array
   *   An array of event listener definitions.
   */
  static function getSubscribedEvents() {
    $events[KernelEvents::CONTROLLER][] = array('onKernelController', 200);
    $events[KernelEvents::TERMINATE][] = array('onKernelTerminate', 200);
    return $events;
  }

Writing the cache on termination was weird, but I guess if someone fiddled with the aliases during the session then...

What I cannot see is where we ever actually did the writeCache. I am not talking about the current code, but from early days when it was first created. (10/2019)

So the flow of our code is something like the following. It should be noted that we have a EventSubscriber that subscribes to the onKernelRequest event, so that we end up coming before RouteNormalizerRequestSubscriber. Our path processors run before our method onKernelRequest. (I would imagine that the core figuring out the route has a really high priority)

flowchart LR
  b["RouteProvider::getRouteCollectionForRequest"]
  c["PathProcessorManager::processInbound"]
  d["PathProcessorAppModule::processInbound"]
  e["AppPathManager::getPathByRequest"]
  f["AppPathManager::initPathData"]
  g[("DataCache")]

  b --> c
  c --> d
  d --> e
  e --> f
  f --> g 

Loading

This kind of mirrors the AliasManager. (This code was all lifted from the AliasManager) The reason we would want caching is the same reason Drupal did with AliasManager - you don't want to hit the DB for every request.

I did check the DB on an ODE and nothing is in the cache_data table, so yeah, as the code says, we are not caching anywhere. So it is kind of an issue that we are skipping the cache, and have been. This could be confirmed by setting a breakpoint in the loadFromStorage method and requesting multiple pages. Each request should fire a loadFromStorage.

So I think we are ok to merge this PR, but we need to make a ticket to fix the not caching. I bet it would speed up requests, especially after cache clears.

[ChadChandlerPS]

Well I will agree that the CacheDecoratorInterface is dead. It was only even "used" in Drupal for the AliasManager and when Aliases became entities things all changed. Technically the interface was never used at all - nowhere in the code did something expect a CDI to be passed in. The only thing that used the methods defined by the CDI was PathSubscriber and that expected a AliasManagerInterface, not a CacheDecoratorInterface...

Now, here is the interesting thing, the PathSubscriber code loaded all aliases onKernelController and then wrote the cache onKernelTerminate. Subsequent requests would read from the cache. Technically they cached each individual route, where our code would store all.

This kind of mirrors the AliasManager. (This code was all lifted from the AliasManager) The reason we would want >caching is the same reason Drupal did with AliasManager - you don't want to hit the DB for every request.

I did check the DB on an ODE and nothing is in the cache_data table, so yeah, as the code says, we are not caching anywhere. So it is kind of an issue that we are skipping the cache, and have been. This could be confirmed by setting a breakpoint in the loadFromStorage method and requesting multiple pages. Each request should fire a loadFromStorage.

So I think we are ok to merge this PR, but we need to make a ticket to fix the not caching. I bet it would speed up requests, especially after cache clears.

Agreed @bryanpizzillo . Suggest maybe wiring it up with a kernel.controller/kernel.terminate subscriber mirroring core's PathAliasSubscriber. I was going to see if you wanted me to create a followup ticket but I saw you created it already. I have a few things we'll want to keep in mind I'll add to the ticket. Nice that you looked at the cache data table and saw nothing. I think. lol

[cgdp-management-server]

ODE Deployment

Code has been deployed to ODE 1153.