Problem/Motivation

In PHP 8.1 \Drupal\path_alias\AliasManager::cacheClear() produces PHP deprecation notices because when it is called $this->cacheKey is NULL and this is then passed to the cache system. See #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves) for test evidence.

Steps to reproduce

Proposed resolution

The easy solution is to wrap $this->cache->delete($this->cacheKey); with a check for $this->cacheKey being set. This, however, makes you realise that the cache flush is flawed. We're setting keys in \Drupal\path_alias\EventSubscriber\PathAliasSubscriber::onKernelController() and the key is not related at all to the $source being passed into the cache flush.

At the moment we're using cache.data as the backend so calling \Drupal\path_alias\AliasManager::cacheClear() without a source probably should not flush all of that.

It turns out that the cache entry is for preloading and it does not matter if it is incorrect - it will fix it self over time. Therefore rather than trying to clear it we should leave it to expire and not try to clear it at all.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3224592-6.patch3.05 KBalexpott
#6 5-6-interdiff.txt2.65 KBalexpott
#5 3224592-5.patch1.69 KBalexpott
#5 2-5-interdiff.txt509 bytesalexpott
#2 3224592-2.patch1.19 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Issue summary: View changes
Status: Active » Needs review
Related issues: +#3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves)
FileSize
3224592-2.patch1.19 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Parent issue: » #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves)
Related issues: -#3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves)

Status: Needs review » Needs work

The last submitted patch, 2: 3224592-2.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Needs work » Needs review
FileSize
2-5-interdiff.txt509 bytes
3224592-5.patch1.69 KB

Here's a fix for the test.

I'm not sure about this fix. After looking at the code and playing with it, given how the cache is used in conjunction with $this->cacheKey I think we’ll need a cache tag and to invalidate them on \Drupal\path_alias\AliasManager::cacheClear if we want to do it properly. But I’m not convinced that that is worth it. We use the cache to preload multiple path aliases for a route - if we miss one path that appears on a page I’m not sure it matters. And we only cache for 24 hours so it will fix itself soon enough. I think maybe we should remove the cache deleting entirely and add a comment as to why it is not here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
FileSize
5-6-interdiff.txt2.65 KB
3224592-6.patch3.05 KB

Discussed with @catch. We agreed that calling cache->delete() here is not necessary as the is a preloading cache that improves performance but it does not matter if it is missing a path or has an extra path. Also since the preloading is an implementation detail of core's AliasManager I've not put this documentation on the interface.

daffie’s picture

daffie CreditAttribution: daffie commented

There is only one place in the alias manager where $this->cache->get() is used. Before this is called there is a test for $this->preloadedPathLookups === FALSE. In the method cacheClear() does the follwing: $this->preloadedPathLookups = [];. Therefor when one first calls the method cacheClear(), the code $this->cache->get() will not be run. Therefor we can remove the call to $this->cache->delete($this->cacheKey) in the method cacheClear().

@alexpott: The only problem that I have is that the name of the method cacheClear() gives to me the implicit expectation that the cache will be cleared, including $this->cache. When the method is called with setting the parameter $source, my implicit expectation would be that all caches for the alias manager would be deleted. If you do not agree with this, then I the patch in comment #6 is for me RTBC.

The method is called from more places then only \Drupal\path_alias\Entity\PathAlias::postSave. The workspaces module does it in Drupal\workspaces\WorkspaceManager::doSwitchWorkspace and the contrib modules fixed_path_alias and pathauto do the same. See: http://grep.xnddx.ru/search?text=-%3EcacheClear%28&filename=.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented

Re #7 - 

+++ b/core/modules/path_alias/src/AliasManager.php
@@ -251,6 +251,10 @@ public function getAliasByPath($path, $langcode = NULL) {
+    // Note this method does not flush the preloaded path lookup cache. This is
+    // because if a path is missing from this cache, it still results in the
+    // alias being loaded correctly, only less efficiently.

Given how the key changes, we cannot flush the cache unless we add a new cache tag to do this. However as this comment indicates, flushing the cache doesn't really change the outcome - the cache is used for preloading and miss or something not being in the cache, does not change the outcome. Imo we have this documented and everything is okay and tested. Also #6 documents that I discussed the operation of this with @catch who was one of the authors of the original code.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented

Also if a path is provided we can't know all the cache entries that will have the path in because the cache key is determined by the request path. So even with a cache tag we'd either have to over zealously clear or not clear at all if $path is provided to this method.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

@alexpott: Thank you for your explanation.

All the code changes look good to me.
The removal of the $this->cache->delete() is documented in the docblock of the method.
For me it is RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Title: \Drupal\path_alias\AliasManager::cacheClear() can cause deprecations on PHP 8.1 and when set to NULL it does not flush the entire cache » \Drupal\path_alias\AliasManager::cacheClear() can cause deprecations on PHP 8.1 and when set to NULL it tries to flush the cache but that's not possible

Fixing the title.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Issue summary: View changes

Fixing issue summary.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Issue tags: +PHP 8.1

  • catch committed 9ac02c4 on 9.3.x 
    Issue #3224592 by alexpott, daffie: \Drupal\path_alias\AliasManager::...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.3.x, thanks!

I wonder if we should open a follow-up to try to remove the cacheClear method and replace everything with cache tags, but seems optional.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

@catch filed #3226090: Deprecate \Drupal\path_alias\AliasManager::cacheClear() in favour of cachetags

Status: Fixed » Closed (fixed)

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