Add an in-memory LRU cache

Just tested, seems to be working neatly.

// Replace the item in first position in the array.

I was mislead by this comment to believe the item was being replaced with the new one - which is not the case and would be nonsense. Maybe something like "Drop the first item - least recently used - to make room for the new item." would be clearer.

So currently entity controllers do like '$cache += $array_of_items;' which is not supported by ArrayObject. I guess a simple method that gets the items array as argument and iterates over them adding could do for now, right? Unless there is some performance impact in adding one by one.

I thought about the second issue. And if you merge both - iterating a number of items above limit - we would most likely get a performance issue because of all the removals.

We could have a private property "$this->bypass" that defaults to FALSE. If it is true, offsetSet() would only call parent::offsetSet(). So this property can be used by an addMultiple() method so that it would handle removals and additions to $this->positions[] on itself and only once, avoiding endless iterations.

Concerning the second issue again, a simple solution would be to just cache a chunk of the maximum size, could be the last n items of the loaded items.

Maybe this should be implemented using the cache backend interface: we could then have an in memory-lru working implementation ready to be used there #1596472: Replace hard coded static cache of entities with cache backends, which would then make #375494: Restrict the number of nodes held in the node_load() static cache a duplicate that should be closed.

One stone, two birds.

Moved catch's LRU implementation to a cache backend.

There's more work to do sort out getMultiple, deletePrefix, expire, garbageCollection, invalidateTags...

This should probably implement the full cache backend interface, I see some missing methods. [EDIT: Sometime I read too quickly sorry.] This also probably would mean it'd need to be decoupled from the LRU class and use its own implementation instead (because of advanced features such as tags etc)?

Merged work from #1637478: Add a PHP array cache backend to fully implement CacheBackend

I think you are doing the work in the wrong way, we should first finish the work of #1637478: Add a PHP array cache backend then continue this issue on a sane basis?

Just for fun and because I'm not always sane :) ... here's is an implementation that extends Pounard's latest patch on #1637478: Add a PHP array cache backend. This will fail tests as that issue is not committed yet.

You're as sane as me, just the code from the other issue definitely wasn't.

This patch is a reroll and extends the GenericCacheBackendUnitTestBase from #1637478: Add a PHP array cache backend

Serious stuff can now happen, good to see this.

Go testbot go!!!!

FYI ... LRUArrayObject.php file is missing @file docblock at the top of the source code.

I'm OK with straight array access, but even better we could just implement it as a normal cache backend and use it into the CacheChain as soon as it can land. Then, the LRU usage would be fully transparent and its interface as a clear as the cache interface itself.

@pounard it is

+++ b/core/lib/Drupal/Core/Cache/MemoryLRUBackend.phpundefined
@@ -0,0 +1,95 @@
+class MemoryLRUBackend extends MemoryBackend {

a CacheBackend :) - because MemoryBackend is

dreditor changed the status... not fair.

If I change it to ArrayAccess I get the following error:

Fatal error: Class Drupal\Component\Utility\LRUArrayObject cannot extend from interface ArrayAccess in /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Component/Utility/LRUArrayObject.php on line 70

Implemented #17

It's extending the new MemoryBackend so it doesn't have to have exactly the same functions... otherwise it'd be implementing the CacheBackend interface.

This might be a really stupid question but does it maybe make sense to go with memory_get_usage() and restrict the cache by memory usage instead of slots? The slots don't really resemble the actual memory usage usually.

#25: That IMHO would only work if several items are added to one slot, but the problem then is that those can be updated independently so I don't see how this could work.

We would have to track the memory usage of an item when we add it to the cache... Like so:

$before = memory_get_usage();
add_item_to_cache($item);
$after = memory_get_usage();
$usage_of_item = $after - $before;

Will get a bit more tricky once we start kicking out items once we hit the limit. But yeah, it's possible by simply using memory_get_usage(). The only question is how well that would perform in general.

So what is nice about #28 is that we could say:

- Store only 8 MB of node objects in the static node cache, but allow a minimum of 8 slots.
- It is getting a little tricky if a reference is dropped however, as the node's object memory would be 0 effectively at time of insertion

But if we assume that its cached when loaded fresh, that should work reasonabiy well and with low memory it

To the patch itself:

Should unset() not update the positions array as well, to free up slots?

1. +++ b/core/lib/Drupal/Core/Cache/StaticCache/LruStaticCache.php
   @@ -0,0 +1,164 @@
   +    // If there are still available slots, decrement the counter.
   +    if ($this->availableSlots) {
   +      $this->availableSlots--;
   +    }
   +    // Otherwise remove the least recently used item.
   +    else {
   +      $key = array_shift($this->positions);
   +      unset($this->cache[$key]);
   +    }
   

   What happens if you set a value which is already in the slots itself? Couldn't you optimize to not store one value twice?

2. +++ b/core/lib/Drupal/Core/Cache/StaticCache/LruStaticCache.php
   @@ -0,0 +1,164 @@
   +    $this->availableSlots++;
   +    $current_position = array_search($cid, $this->positions);
   +    unset($this->positions[$current_position]);
   +    parent::delete($cid);
   

   What happens if the item doesn't exist yet. Shouldn't we check whether there is an item first?

3. +++ b/core/lib/Drupal/Core/Cache/StaticCache/StaticCache.php
   @@ -0,0 +1,65 @@
   +/**
   + * Defines a static cache implementation.
   + *
   + * Stores cache items in memory using a PHP array.
   + *
   + * @ingroup cache
   + */
   +class StaticCache extends MemoryBackend implements StaticCacheInterface {
   

   It would be nice to explain the difference between MemoryBackend and StaticCache

4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -104,9 +105,12 @@ class ConfigEntityStorage extends EntityStorageBase implements ConfigEntityStora
   +    $static_cache = isset($static_cache) ? $static_cache : \Drupal::service('static_cache');
   
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -43,9 +44,12 @@
   +    $static_cache = isset($static_cache) ? $static_cache : \Drupal::service('static_cache');
   
   +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -73,18 +74,28 @@
   +  public function __construct(EntityTypeInterface $entity_type, StaticCacheInterface $static_cache = NULL) {
   ...
   +    $this->staticCache = isset($static_cache) ? $static_cache : \Drupal::service('static_cache');
   

   Do we need the BC in every of those 3 levels?

With #4, you could extend from any one of those classes individually, so I think we do?

Won't ConfigEntityStorage and ContentEntityStorageBase fallback to EntityStorageBase, given that we pass along $static_cache to it?

Is this really a jenkins issue?

09:03:13 Slave went offline during the build
09:03:13 ERROR: Connection was broken: java.io.EOFException
09:03:13 	at org.jenkinsci.remoting.nio.NioChannelHub$3.run(NioChannelHub.java:613)
09:03:13 	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
09:03:13 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
09:03:13 	at hudson.remoting.SingleLaneExecutorService$1.run(SingleLaneExecutorService.java:112)
09:03:13 	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
09:03:13 	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
09:03:13 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
09:03:13 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
09:03:13 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
09:03:13 	at java.lang.Thread.run(Thread.java:745)
09:03:13 

I was simply thinking of this ...

Re-rolled.

I recently found this implementation of a in-memory LRU cache: https://github.com/aws/aws-sdk-php/blob/master/src/LruArrayCache.php and it seems a bit simpler than the one from the current patch. We would simply work on the \Drupal\Core\Cache\MemoryBackend::$cache variable, without the need for $positions and $availableSlots. What do you think?

Nice, that looks much better!

Just a few more nitpicks left:

1. +++ b/core/lib/Drupal/Core/Cache/MemoryCache/LruMemoryCache.php
   @@ -0,0 +1,111 @@
   +use Drupal\Core\Cache\Cache;
   +
   +
   +/**
   
   +++ b/core/tests/Drupal/Tests/Core/Cache/LruMemoryCacheTest.php
   @@ -0,0 +1,120 @@
   +  }
   +
   +
   +}
   

   Extra empty lines here :)

2. +++ b/core/tests/Drupal/Tests/Core/Cache/LruMemoryCacheTest.php
   @@ -0,0 +1,120 @@
   +   * Test getting, setting and deleting items from the static cache.
   +   */
   +  public function testGetSetDelete() {
   

   We should also specify which methods are covered by this test.

3. +++ b/core/tests/Drupal/Tests/Core/Cache/LruMemoryCacheTest.php
   @@ -0,0 +1,120 @@
   +  public function testInvalidate() {
   

   Should have

   @covers ::invalidate
   @covers ::invalidateMultiple
   

Also, I don't see any test coverage for invalidateTags(). Do you think it would be useful or not really?

I don't feel strongly about covering invalidateTags(), so I think this is ready :)

+++ b/core/lib/Drupal/Core/Cache/MemoryCache/LruMemoryCache.php
@@ -0,0 +1,110 @@
+    $diff = count($this->cache) - $this->allowedSlots;
+    // Remove items from the cache entry until it's back to the allowed number
+    // of slots.
+    if ($diff > 0) {
...
+      while ($diff) {
...
+        $diff--;

Under which circumstances could diff be bigger than 1?

+++ b/core/lib/Drupal/Core/Cache/MemoryCache/LruMemoryCache.php
@@ -0,0 +1,110 @@
+   *   The number of slots to allocate for items in the cache.

(Optional) - I'd default this to 100 and always set it. That way we'd lose a condition and the default value is exposed by IDEs when you construct.

* Do we have an issue where we're actually going to use this for the entity cache (there's the one about node_load() mentioned, we could use that I guess?)
* Would 100 make sense for the entity cache.. we have to consider that this is for all entity types in the system AFAIK, so 100 isn't *that* much, especially if we start to statically cache config entities more than we do now? If not, should increase the default and/or expose this as a container parameter/service argument (possibly both?)
* Issue summary says this hasn't been profiled yet, do we need to do that? Or will we profile this instead when used in action as the entity cache backend?

1. +++ b/core/lib/Drupal/Core/Cache/MemoryCache/LruMemoryCache.php
   @@ -0,0 +1,110 @@
   +      // Move the item to the end of the array.
   +      unset($this->cache[$cid]);
   +      $this->cache[$cid] = $cached;
   

   How about expand the documentation to tell why we are dong that. "Move the item to the end of the array, so it'll be cleaned up last".

2. +++ b/core/lib/Drupal/Core/Cache/MemoryCache/LruMemoryCache.php
   @@ -0,0 +1,110 @@
   +    // Remove items from the cache entry until it's back to the allowed number
   +    // of slots.
   +    if ($diff > 0) {
   +      reset($this->cache);
   +      while ($diff) {
   +        unset($this->cache[key($this->cache)]);
   +        next($this->cache);
   +        $diff--;
   +      }
   +    }
   

   May I ask why this code cannot just be using array_slice?

Array slice makes a copy of the array for big arrays it could be a memory issue but array_splice could work-around that though. Overall agree with #68.2

We can't use array_splice() either because it doesn't preserve numeric keys.

I think going over the limit could lead to out of memory errors, right? So I agree that we should keep it simple for now and stick to the limit until we have some evidence that going over the limit is actually useful at some point :)

This looks great, what's missing for it to be added?

Thanks, this is awesome and useful. Two possible enhancements:

1. +++ b/core/lib/Drupal/Core/Cache/MemoryCache/LruMemoryCache.php
   @@ -0,0 +1,109 @@
   +      reset($this->cache);
   +      unset($this->cache[key($this->cache)]);
   

   https://www.php.net/manual/en/function.array-key-first.php it has a polyfill, too.

2. +++ b/core/lib/Drupal/Core/Cache/MemoryCache/LruMemoryCache.php
   @@ -0,0 +1,109 @@
   +      unset($this->cache[$cid]);
   +      $reversed = array_reverse($this->cache);
   +      $reversed[$cid] = $item;
   +      $this->cache = array_reverse($reversed);
   

   If I gather correctly you want to move the item to end and it is already unset. What about $this->cache += [$cid => $item] instead of the double array_reverse https://3v4l.org/82ZV8

+++ b/core/lib/Drupal/Core/Cache/MemoryCache/LruMemoryCache.php
@@ -0,0 +1,109 @@
+      reset($this->cache);
+      unset($this->cache[key($this->cache)]);

https://www.php.net/manual/en/function.array-key-first.php it has a polyfill, too.

array_key_first() is only available for PHP 7 >= 7.3.0.

------

+  public function invalidate($cid) {
+    parent::invalidate($cid);
+    // Move the item to the least recently used position if it's not already
+    // there. This cannot use array_unshift() because it would reindex an array
+    // with numeric cache IDs.
+    if (isset($this->cache[$cid])) {
+      $item = $this->cache[$cid];
+      unset($this->cache[$cid]);
+      $reversed = array_reverse($this->cache);
+      $reversed[$cid] = $item;
+      $this->cache = array_reverse($reversed);
+    }
+  }

Why invalidating an entity makes it the least recently used one? If the invalidated entity is loaded by setting $allow_invalid = TRUE, then it will become the most recently used one. Therefore I don't really see a reason for the overhead of reordering the entities when they are being invalidated. Yes, one could argue that we want to remove first the invalidated entities when there is no place left, but then this isn't pure LRU implemention anymore and I can see people complaining about this inconsistency. I would rather leave the invalidation part away from the LRU implemention.

Sorry I didn't elaborate enough: array_key_first is 7.3.0 but that's okay because the manual gives us a very simple polyfill and if we add said polyfill then all code under Drupal can enjoy that useful feature before 7.3 is required and don't need to change once it is. (As a footnote, I really hope we won't rush with 7.3 requirement, that serialize business is very problematic.)

If I gather correctly you want to move the item to end and it is already unset. What about $this->cache += [$cid => $item] instead of the double array_reverse https://3v4l.org/82ZV8

I am not sure if it is true. here is current vs +=: https://3v4l.org/JYCGa

Ah. Thanks for the demo. $this->cache = [$cid => $item] + $this->cache is still better than two array_reverses :) bonus: you don't need the unset. https://3v4l.org/6uEX1

#82: +1. nice!

Regarding #67 / #67 for the number of keys, lots of D8 sites are using Paragraphs, which tends to cause an explosion of the number of entities actually loaded on node loads. So 100, or even 300, is probably erring on the low side. I'd likely add at least one zero, especially if this is only an optionally switchable service for drush and other non-default environments, which have higher memory resources than the minimal web-oriented defaults.

+++ b/core/lib/Drupal/Core/Cache/MemoryCache/LruMemoryCache.php
@@ -0,0 +1,109 @@
+  protected $allowedSlots = 100;

100 could be removed here as constructor has default 300.

Is there a new service that will allow custom code to use that? For example cache.memory.lru. I was not able to see a service definition on the patch.

What are the plans to use the LRU for, except the discussed entity cache?

Have you considered the new PHP's (7.4) weak references?
This way GC will handle the clean-up when there is memory pressure.

@ndobromirov nice point about php version, it sounds like needs polyfill but mostly reminds me about "hot cache preload" discussions years ago, means it very probably will be implemented in contrib first like chainedFast and so on

While WeakReference would be nice, I can't imagine how that could be polyfilled so I think that is not useful for core for the time being (7.4 is not even out yet).

Follow up for weak references probably should go into 9.1

Does minimal changes to respond to the latest feedback on the patch. Doesn't address concerns with invalidate from @hchonov.

@catch there's implementation in symfony which already in dependencies https://github.com/symfony/polyfill/issues/228#issuecomment-591843988

We also depend on 7.3 at this point in the drupal core development lifecycle, so even better.

In 8.1 under discussion [RFC] CachedIterable (rewindable, allows any key&repeating keys)

Since we're back here I did a review. Everything applied cleanly since its just new classes

I think I echo a lot of other reviews in that it feels like the array mangling could be optimized but pretty sure it would need to be decoupled from the MemoryCache and add a lot of complexity. I wouldn't hold up committing it on that since there's no core usage and its sat long enough. We can work within the interface with faster implementations if someone comes up with something clever.

+++ b/core/lib/Drupal/Core/Cache/MemoryCache/LruMemoryCache.php
@@ -0,0 +1,106 @@
+      return parent::set($cid, $data, $expire, $tags);
+    }
+    $return = parent::set($cid, $data, $expire, $tags);

These are void methods so we don't need to worry about the returns. I don't see any tests of the returns so easy fix.

I removed the returns as mentioned in #101.

Thanks @JeroenT for applying the suggestions. I reviewed #105 and it looks that we're on a good track here.

Some notes:

1.

<?php
+  protected $allowedSlots = 100;
[...]
+  public function __construct(int $slots = 300) {
[...]
?>

Can we please be more consistent here and at least have one default value? I'm fine with removing the default value of the property here.

I'd recommend to see that added LRU in-memory cache only as an additional goodie that site owners may use to replace existing caches, as they are available as services and thus replaceable as such anyway. Changing existing ones could mean major side effects on existing installations, and those who had problems with memory overflows, they mostly have it solved already on their own, also maybe using a custom strategy that works well for their site, but the LRU could destroy it.

2.

<?php
$diff = count($this->cache) - $this->allowedSlots;
?>

Guess this was already discussed, still the count doesn't feel right to me (unless PHP is already heavily optimized when counting arrays, then it's fine). I'm thinking of having large lists with super-tiny items, where I'd prefer an increment property and safely rely on it (I mean the cache property is protected, so what's the deal with not having a simple incement?).

re #106
1. I noticed that too. I think you just don't provide the default on the property if its always set in the constructor.
2. Its a function call but my understanding (and reading of the source just now) is that the function is just going to return the value of a property on the zval. A lot of time we try to avoid count() and try to use empty() because of that function call but being on the write side it seems appropriate here.

I'd agree when the count would happen within setMultiple, but it's being called whenever a single item is being added. It's probably only of a minor concern, but would be great to have it as efficient as possible right away so that it doesn't hurt also when handling lots of (tiny) items.

Does it still makes sense?

I think we are missing a return statement in the if-condition.

<php
+++ b/core/lib/Drupal/Core/Cache/MemoryCache/LruMemoryCache.php
+    // If the item is already in the cache, just move it to the first position.
+    if (isset($this->cache[$cid])) {
+      unset($this->cache[$cid]);
+      parent::set($cid, $data, $expire, $tags);
+    }
+    parent::set($cid, $data, $expire, $tags);

Updated the patch from #105 in a MR

- Move allowedSlots property declaration to constructor; replaces mismatched default value on property (#106)
- Added missing early return in set() (#113)
- Override setMultiple() to only count() extra items once before removal (#108)
- Code style fixes

Just cross-referencing with another old issue: https://www.drupal.org/project/drupal/issues/1961934#comment-7274730

Back when I raised that I was seeing a pattern of entity cache problems, and observed that a LRU approach was similarly problematic in the circumstances in question ("With LRU and limited memory, you can easily throw out everything you want to keep"), so I proposed a way of providing hints to the cache to enable it to be smarter.

I haven't tried to catch up with what's currently happening in this issue, so apologies in advance if it's not relevant any more; but seeing as how there's current activity on this LRU caching issue, I thought I'd add a link in case it was still a going concern.

The example scenario was:

* You are looping through a potentially-large list of entities (e.g. apache solr doing an indexing cron run).
* Each entity must be loaded for indexing, but you have no reason to expect anyone else to be requesting it in the near future.
* Conversely, the entity load cache is full of things which users have been actively requesting, so we want to retain all of those entities.
* So for each indexing loop iteration we want to ensure that the indexed entity only ends up in the load cache if it was already there beforehand. Any freshly-loaded entities in the indexing loop should be discarded from memory, uncached; because despite being "more recently used" we don't need them -- caching them all would merely push out a lot of much-more-useful cache entries.

@jweowu we're talking about static memory caches here so what other users are doing is not really relevant. A static memory cache is empty at the start of every request.

Very true, "other users" are only relevant to the persistent caches (I was considering both in the other issue).

I don't believe that renders the notion irrelevant, though. After all, no cache-expiry scheme has any advantage over any other if the cache doesn't fill up. We're only concerned about what happens when, for whatever reason, the cache fills up, and my suggestion is that giving the programmer some control over what should be flushed and what should be kept in those situations would be an improvement, and that a simple LRU approach isn't always going to be the best approach.

Pushing on this one again. The entity static cache filling up on long running jobs is an issue I run into quite often work around and then when I hit it again I get sad that we're get to fix this.

I'll have a quick look myself to see if I can +1 this.

Okay so the code will work, I really like the thorough test coverage and the fact that we uncovered MemoryCache itself didn't have a test extending GenericCacheBackendUnitTestBase yet. Only remark I have in general is that some methods are dedicated to numeric keys while other methods cover both string and numeric keys in one go. But don't let that detract from the overall excellent coverage.

I found a tiny nit regarding the word pidgin and think the test could benefit from renaming the assertion and an extra comment or two. This is why I'm setting back to NW.

The comment about moving the shared logic I'll leave up to you to decide, as MemoryBackend also seems to copy-paste things across methods occasionally. So I wouldn't be too bothered if we copy-paste a few lines of code here too. So feel free to RTBC without addressing this.

Now that this introduces another memory cache, would it be prudent to add a line to MemoryCache and LruMemoryCache's docblocks to tell people they should declare these bins using the cache.bin.memory tag?

I fixed 3/4 of the comments on the MR. There was one I didn;'t think needed doing.

Re the cache.bin.memory tag - interestingly the entity.memory_cache does not use this tag and it's not really managed like other cache bins or memory cache bins. I think we should consider adding documentation about somewhere but I don't think it should be in this issue. I also think we could consider defaulting to a LRU cache for all memory caches.

One final nit, but can be accepted upon commit.

I agree that we should create a follow-up for checking if we have sufficient documentation re cache.bin.memory and why entity storages don't use it like that.

I tested this code on a long running batch process from the Entity Usage module - to build the entity_usage table from a large site 30,000+ nodes and 200000+ paragraphs using a module I made with this code - see https://www.drupal.org/project/entity_lru_cache - and it makes a massive difference. Memory no longer spirals to 90% of VM I'm running the job in and everything is a bit quicker too.

Oh wow, that's great. We should consider making it the default then, no?

@kristiaanvandeneynde see #3498154: Use LRU Cache for static entity cache

I'm running a migration that usually takes 40 hours ish.
As a control I first am running it with attemptMemoryReclaim commented out on Drupal 11.1.0

I will add info from ps aux --sort -rss |grep 'drush' too periodically.
I will update periodically.
It's several migrations chained
11280/81350 8min 43s/1hr 2min 24MiB
24038/81350 22min 37s/1hr 15min 26MiB
41000/81350 40min/1hr 19min 26.5MiB
Memory 23.3% VSZ690000 RSS107000

@alexpott on Slack said I should see a difference almost immediately, I will kill it and compare after adding this:

composer require drupal/entity_lru_cache
drush en entity_lru_cache -y

I am running it with attemptMemoryReclaim commented out on Drupal 11.1.0 and the above module enabled.
11000/81350 7min 5s/52min 30s 24Mib
20.7%
25000/81350 22min 8s/1hr 11min 26.0MiB
22.4%
71000/ 1hr 10min/1hr 18min 31MiB
22.3%

Gonna wait for some of the bigger migrations.

We've gotten to the bigger migration, this is with the LRU cache
1682/1146871 21min/10days3hrs 710MiB
82%
2134/1146871 41min/15days 12hrs 732MiB
91.8%
Ok I killed it when it got to 27 days.

I am rerunning 11.x base just the long migration.

Discussed sizing a bit @catch and we agreed to remove the default as any code that uses a LRU cache should consider the number of slots for the use-case.

Since my last comment I ran the migration on production with and without the LRU cache. Both took 39 hours.

@alexpott mentioned this is a positive result since it's the same, this update will either improve or do nothing, my migration helps prove if it's not needed it does not make the situation worse.

Wonder if we could go with LFU cache instead. Will the implementation be more complex?

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed the PHPStan issue.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed the sniffer error using a suggestion.

Committed 4a3fbdb and pushed to 11.x. Thanks!