Problem/Motivation

drupal_static() will be deprecated in #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset().

Proposed resolution

Switch to class protected property.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

Original report

Child of #1577902.

BookManager::bookSubtreeData
BookManager::bookTreeAllData
BookManager::doBookTreeBuild

have calls to drupal_static(). These should be replaced with protected properties.

CommentFileSizeAuthor
#58 2412669-55.patch18.57 KBclaudiu.cristea
#57 2412669-57.patch18.57 KBclaudiu.cristea
#57 2412669-57.interdiff.txt6.58 KBclaudiu.cristea
#55 2412669-55.patch18.57 KBclaudiu.cristea
#55 2412669-55.interdiff.txt1.72 KBclaudiu.cristea
#51 2412669-51.patch18.23 KBclaudiu.cristea
#51 2412669-51.interdiff.txt579 bytesclaudiu.cristea
#47 2412669-47.interdiff.txt714 bytesclaudiu.cristea
#47 2412669-47.patch18.39 KBclaudiu.cristea
#40 2412669-40.interdiff.txt1.48 KBclaudiu.cristea
#40 2412669-40.patch18.43 KBclaudiu.cristea
#39 2412669-39.interdiff-to-32.txt19.39 KBclaudiu.cristea
#39 2412669-39.patch18.43 KBclaudiu.cristea
#36 2412669-36.patch20.35 KBclaudiu.cristea
#36 2412669-36.interdiff.txt15.52 KBclaudiu.cristea
#32 2412669-32.patch14.02 KBclaudiu.cristea
#32 2412669-32.interdiff.txt1.13 KBclaudiu.cristea
#28 2412669-28.interdiff.txt11.06 KBclaudiu.cristea
#28 2412669-28.patch12.89 KBclaudiu.cristea
#26 2412669-26.patch9.6 KBclaudiu.cristea
#26 2412669-26.interdiff.txt5.54 KBclaudiu.cristea
#23 2412669-23.interdiff.txt4.39 KBclaudiu.cristea
#23 2412669-23.patch5.03 KBclaudiu.cristea
#22 2412669-22.patch4.31 KBclaudiu.cristea
#22 2412669-22.interdiff.txt2.35 KBclaudiu.cristea
#20 2412669-20.interdiff.txt3.08 KBclaudiu.cristea
#20 2412669-20.patch4.3 KBclaudiu.cristea
#17 2412669-17.interdiff.txt3.57 KBclaudiu.cristea
#17 2412669-17.patch4.22 KBclaudiu.cristea
#5 2422669-5.patch4.66 KBJulfabre
#5 interdiff.txt3.62 KBJulfabre
#1 2412669-1.patch4.39 KBsidharrell

Issue fork drupal-2412669

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sidharrell’s picture

Status: Needs work » Needs review
FileSize
4.39 KB

Please review with some kindness. My first time actually changing code on an issue. Thanks :)

ParisLiakos’s picture

thanks for working on this!

with drupal_static() those data where stored in separate places. now they are stored in the same property.
they should probably be stored in different properties

eg
bookLinksAll
bookLinksTree
bookLinksSubTree

meramo’s picture

Status: Needs review » Needs work

Changing status, obviously needs more work

Julfabre’s picture

Assigned: Unassigned » Julfabre
Issue tags: +drupaldevdays

I'm working on it for drupal dev day's

Julfabre’s picture

Status: Needs work » Needs review
FileSize
3.62 KB
4.66 KB

We refactor for 3 differents properties.

maartendeblock’s picture

Status: Needs review » Reviewed & tested by the community

Tested, the book functionality still works fine.

alexpott’s picture

Status: Reviewed & tested by the community » Postponed
Issue tags: +D8 patch release target

This is not permitted as per the beta policy guidelines and I don;t feel that it meets the requirement to be committed under committer discretion - postponed to after 8.0.0

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

claudiu.cristea’s picture

Version: 8.6.x-dev » 8.7.x-dev
Assigned: Julfabre » Unassigned
Issue tags: -Novice, -drupaldevdays, -D8 patch release target

I guess this can be un-postponed.

claudiu.cristea’s picture

Status: Postponed » Needs work
claudiu.cristea’s picture

claudiu.cristea’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
4.22 KB
3.57 KB

Fixed the IS. Rerolled an performed only some cosmetic changes. Let's see the bots.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Seems pretty straight-forward, only reason this wasn't committed 3 years ago is that it was too late during the beta phase.

catch’s picture

Status: Reviewed & tested by the community » Needs work

ParisLiakos' review, which was followed up in #5 looks correct to me - we're changing the semantics of the static cache here, going from a cache per method to a single cache item.

The cid logic should mean we don't get collisions, but should we make this a nested array with distinct keys or something? So $this->tree['link'][$cid] or something?

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
4.3 KB
3.08 KB

OK, namespaced the cache, even, IMO, the cid prefix was safe enough.

Status: Needs review » Needs work

The last submitted patch, 20: 2412669-20.patch, failed testing. View results

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
2.35 KB
4.31 KB

Ok.

claudiu.cristea’s picture

FileSize
5.03 KB
4.39 KB

I'm still not comfortable mixing persistent cache IDs with memory cache IDs. In memory, we'll have kind of redundancy in keys. So, I'm now using the vars:

  • $key: for memory cache.
  • $cid: for persisted cache.

and they are different. I.e. $key doesn't contain the book-links:subtree-cid: prefix.

Some persistent cache IDs have a different pattern now. As an effect, on an existing site, the existing cache will not be retrieved and fresh cache entries will be created. I don't see the need of a "cache clear" post update script as, on the next cache rebuild, the stale entries will be removed anyway and they cannot interfere. I think this is worth, as now the structure of cache is more clear.

Mile23’s picture

Just some almost-nits:

  1. +++ b/core/modules/book/src/BookManager.php
    @@ -74,6 +74,13 @@ class BookManager implements BookManagerInterface {
    +  /**
    +   * Memory cache for built trees.
    +   *
    +   * @var array
    +   */
    +  protected $cache = [];
    
    @@ -482,17 +489,16 @@ public function deleteFromBook($nid) {
    +    if (!isset($this->cache['all'][$key])) {
    
    @@ -681,17 +685,18 @@ protected function doBookTreeBuild($bid, array $parameters = []) {
    +    if (!isset($this->cache['tree'][$key])) {
    

    Maybe document the literal keys and their meaning in the $this->cache docblock?

  2. +++ b/core/modules/book/src/BookManager.php
    @@ -681,17 +685,18 @@ protected function doBookTreeBuild($bid, array $parameters = []) {
    +      $cache = \Drupal::cache('data')->get($cid);
    ...
    +        $this->cache['tree'][$key] = $cache->data;
    

    Maybe rename to $data_cache or something so that the names are distinct?

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

claudiu.cristea’s picture

FileSize
5.54 KB
9.6 KB

Fixed #24.

Now an aspect that came after I learn from deprecating lots of drupal_static() & Co.

What if a 3rd party code did this?

drupal_static_reset('Drupal\book\BookManager::bookTreeAllData');

Until we deprecate, that is a legit usage of our API because drupal_static() and drupal_static_reset() are Drupal public API. So, we have to cover this possibility. Thus we need to add a cache reset alternative and to deprecate the usage of drupal_static_reset() for this specific cases. I've added a public resetCache() method but not on the interface as the class internal cache is an implementation detail. It has to be public but not part of the contract.

I've also triggered deprecation errors when using drupal_static_reset() for these specific static caches. I did the same for other issues from this [META] that's why I use internationally the switch () {...} statement (to be extended).

I've added also tests and a CR to outline this deprecation: https://www.drupal.org/node/3039439.

kim.pepper’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/book/src/BookManager.php
    @@ -681,17 +690,18 @@ protected function doBookTreeBuild($bid, array $parameters = []) {
    +      $persistent_cache = \Drupal::cache('data')->get($cid);
    

    Inject?

  2. +++ b/core/modules/book/src/BookManager.php
    @@ -707,11 +717,11 @@ protected function doBookTreeBuild($bid, array $parameters = []) {
    +      \Drupal::cache('data')->set($cid, $data, Cache::PERMANENT, ['bid:' . $bid]);
    

    Inject?

  3. +++ b/core/modules/book/src/BookManager.php
    @@ -1153,10 +1162,17 @@ public function bookSubtreeData($link) {
    +    $this->cache = [];
    

    Do we need to reset the cache backend too?

  4. +++ b/core/modules/book/tests/src/Unit/BookManagerTest.php
    @@ -58,12 +59,18 @@ class BookManagerTest extends UnitTestCase {
    +    \Drupal::setContainer($container);
    

    Should this be a kernel test?

  5. +++ b/core/modules/book/tests/src/Unit/BookManagerTest.php
    @@ -115,4 +122,34 @@ public function providerTestGetBookParents() {
    +   * @group legacy
    

    Should deprecation tests be in a separate legacy test class?

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
12.89 KB
11.06 KB

Fixed #27.1, 2, 4, 5.

Regarding #27.3: I think we should limit the reset to internal memory cache. The same does the entity storage resetCache(). The reason is that the persistent cache is accessible from outside the class.

Status: Needs review » Needs work

The last submitted patch, 28: 2412669-28.patch, failed testing. View results

catch’s picture

Two things with this:

1. drupal_static was mainly added for testing purposes. So drupal_static_reset() while it's an API for resetting a static cache you control, isn't something that can be relied on for any static cache. I don't think we need to provide bc here. One way to find out if anyone's using this would be to grep contrib though.

2. If we really do need an external reset on a class property, then using a memory cache property allows the cache to be resettable without adding a method. https://www.drupal.org/node/2973262

claudiu.cristea’s picture

If we really do need an external reset on a class property, then using a memory cache property allows the cache to be resettable without adding a method.

@catch, but that would mean defining a new service, right? Because I don't understand why the existing, entity.memory_cache, was named to refer only to entities, while looking to the implementation seems usable for all kind of memory caches.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
14.02 KB

Fixing the test.

#30.1: I think is safe to keep the BC for cache reset. We have the same issue in other bunch of issues, so I would standardize this for reprecating usages of drupal_static_reset().

#30.2: Waiting for a reply on #31.

andypost’s picture

Does it make sense to create a special memory cache service for this kind of cache?
Each time I see this kind if caching (in properties) I see we bring more state to services...
It makes core more far from running in php-pm and so on

andypost’s picture

Forum manager also caches some data but use some magic for serialisation

claudiu.cristea’s picture

@catch, @andypost we should get first a decision in #3047289: Standardize how we implement in-memory caches.

claudiu.cristea’s picture

FileSize
15.52 KB
20.35 KB

Here I'm trying something crazy: coupling a persistent cache backend with a memory cache backend and abstracting the memory > persistent fallback that we're using in a lot of places in core and contrib. It this eligible to be standardized at core level? Have no idea.

Berdir’s picture

+++ b/core/modules/book/src/Cache/BookCache.php
@@ -0,0 +1,162 @@
+/**
+ * Cache service using a memory cache and falling back to a persistent cache.
+ */
+class BookCache implements CacheBackendInterface {

https://md-systems.github.io/drupal-8-caching/#/4/4

You might be able to use a BackendChain for this, I guess it should work by defining a memory service, injecting both and creating the chain in the constructor.

Status: Needs review » Needs work

The last submitted patch, 36: 2412669-36.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
18.43 KB
19.39 KB

@Berdir, thank you for the hint. Using BackendChain should be a pattern everywhere where we need static doubled by persistent cache. Great!

The interdiff is against #32.

claudiu.cristea’s picture

FileSize
18.43 KB
1.48 KB

Some small improvements.

claudiu.cristea’s picture

kristiaanvandeneynde’s picture

+++ b/core/modules/book/book.services.yml
@@ -30,10 +30,25 @@ services:
+  book.memory_cache:
+    class: Drupal\Core\Cache\MemoryCache\MemoryCache
+    public: false
+  book.cache:
+    class: Drupal\Core\Cache\BackendChain
+    # The service argument will be not mandatory and deprecated in #3061117.
+    # @see https://www.drupal.org/project/drupal/issues/3061117
+    arguments: ['']
+    calls:
+      - [appendBackend, ['@book.memory_cache']]
+      - [appendBackend, ['@cache.data']]
+    tags:
+      # This tag ensures that Drupal's cache_tags.invalidator service
+      # invalidates also this cache data.
+      - { name: cache.bin }

Why are we all of the sudden doing this in core? It's not how you're supposed to define caches. I seriously don't see why we simply can't do it the right way: Define a factory and use said factory.

  cache.backend.memory_no_serialize:
    class: Drupal\Core\Cache\MemoryCacheFactory
  cache.static_no_serialize:
    class: Drupal\Core\Cache\CacheBackendInterface
    tags:
      - { name: cache.bin, default_backend: cache.backend.memory_no_serialize }
    factory: cache_factory:get
    arguments: [static_no_serialize]

namespace Drupal\Core\Cache;

use Drupal\Core\Cache\MemoryCache\MemoryCache;

class MemoryCacheFactory implements CacheFactoryInterface {

  /**
   * Instantiated memory cache bins.
   *
   * @var \Drupal\Core\Cache\MemoryCache\MemoryCache[]
   */
  protected $bins = [];

  /**
   * {@inheritdoc}
   */
  public function get($bin) {
    if (!isset($this->bins[$bin])) {
      $this->bins[$bin] = new MemoryCache();
    }
    return $this->bins[$bin];
  }

}
claudiu.cristea’s picture

@kristiaanvandeneynde, I had a discussion with @catch and there was decision to not create a factory for memory cache. In fact it’s the same thing. A factory doesn’t do anything more.

kristiaanvandeneynde’s picture

What's the reasoning behind this? I can see multiple modules wanting to use MemoryCache, having a factory would only simplify this.

claudiu.cristea’s picture

@kristiaanvandeneynde

book.memory_cache:
  class: Drupal\Core\Cache\MemoryCache\MemoryCache
tags:
  - { name: cache.bin }

VS.

cache.static_no_serialize:
  class: Drupal\Core\Cache\CacheBackendInterface
  tags:
    - { name: cache.bin, default_backend: cache.backend.memory_no_serialize }
  factory: cache_factory:get
  arguments: [static_no_serialize]

Where is the simplification?

However, I proposed a factory in #3047289: Standardize how we implement in-memory caches but, indeed, there's no big win using a factory as you can see in the YAML snippets above.

catch’s picture

Current entries for memory cache services look like this:

entity.memory_cache:
    class: Drupal\Core\Cache\MemoryCache\MemoryCache

I don't really see how it could be made less complex than that, it's literally two lines.

I don't think we should be using MemoryCache as an actual cache backend though, because it does not confirm to the same behaviour as cache backends in regards to serialize/unserialize - or at least we should actively decide why that's not a problem. This is the difference between Drupal\Core\Cache\MemoryCache\MemoryCache and (for testing purposes only) Drupal\Core\Cache\MemoryBackend.

claudiu.cristea’s picture

FileSize
18.39 KB
714 bytes

Other minor fixes.

kristiaanvandeneynde’s picture

@catch, I agree that it is less complex that way.

The reason I'm advocating for cache factories is because we have good use cases for it. Ranging from swapping out a single backend and magically having all code that uses said backend using the new one to injecting the factory into a class rather than the cache itself so that the implementation can decide on what bin to use (e.g. render cache does this with $element['#cache']['bin']).

I don't think we should be using MemoryCache as an actual cache backend though, because it does not confirm to the same behaviour as cache backends in regards to serialize/unserialize

This I couldn't agree more with. We now have two classes doing exactly the same, except that one always returns the same copy whereas the other returns clones but is significantly slower by doing so. It's time we properly decide what to do with MemoryCache. As it stands, entity handlers now use it but still have their own way of clearing caches rather than using cache tags.

Please note JSON API uses MemoryCache because it's way faster than MemoryBackend. So perhaps we should rename the two to indicate what they do? Warning: crappy name suggestions ahead. MemoryBackend and MemoryBackendNoClones? Either way, we should discuss this further in a separate issue. We already have #2973286: [PP-1] Clean up MemoryCache and MemoryBackend naming issues and #3016690: Audit usages of cache.static, but maybe we need a third one to discuss whether we need a factory and a fourth one to discuss what we actually want to do with MemoryCache altogether?

@claudiu.cristeasorry for derailing this issue :(

claudiu.cristea’s picture

@kristiaanvandeneynde, please move this discussion in the proper issue.

kristiaanvandeneynde’s picture

@claudiu.cristea That's what I just said :D

claudiu.cristea’s picture

FileSize
579 bytes
18.23 KB

#3061117: Make optional and deprecate the BackendChain constructor parameter was merged. Removing unneeded argument from service.

daffie’s picture

Status: Needs review » Needs work

The patch looks good to me. Only 1 nitpick:

+++ b/core/modules/book/src/BookManager.php
@@ -482,34 +511,37 @@ public function deleteFromBook($nid) {
-    $cid = 'book-links:' . $bid . ':all:' . $nid . ':' . $language_interface->getId() . ':' . (int) $max_depth;
...
+    $cid = implode(':', ['book-links', $bid, $nid, $language_interface->getId(), (int) $max_depth]);

I am missing the "all" in the new version.

claudiu.cristea’s picture

Status: Needs work » Needs review

@daffie that is superfluous. The uniqueness of the CID is ensured without "all". It cannot collide with other CIDs.

Status: Needs review » Needs work

The last submitted patch, 51: 2412669-51.patch, failed testing. View results

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
1.72 KB
18.57 KB

Oops, new test changes.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

claudiu.cristea’s picture

FileSize
6.58 KB
18.57 KB

D 8.9.x

claudiu.cristea’s picture

Version: 8.9.x-dev » 8.8.x-dev
FileSize
18.57 KB

As I've discussed in the chat with @larowlan, it's not possible to deprecate in 8.9.x and remove in 9.0.x. That's why #57 should be ignored. I'm reposting #55 for review and switching to 8.8.x so that the committer could decide if it can be accepted.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tedbow’s picture

Version: 8.9.x-dev » 9.3.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll

I think this needs to happen first on 9.3.x now, correct? If so it needs a re-roll

claudiu.cristea’s picture

Title: Remove drupal_static from BookManager » 2412669-bookmanager
Status: Needs work » Needs review
Issue tags: -DevDaysTransylvania, -Needs reroll

Rerolled, adapted deprecation messages. I moved to a MR

claudiu.cristea’s picture

Title: 2412669-bookmanager » Remove drupal_static from BookManager
tedbow’s picture

Status: Needs review » Needs work

Needs work for comments in Gitlab

claudiu.cristea’s picture

Status: Needs work » Needs review
tedbow’s picture

@claudiu.cristea thanks for addressing my points in the MR. all those changes look good!

I haven't been able to review the majority of the changes in core/modules/book/src/BookManager.php yet though and comments here. Will try to soon

Berdir’s picture

Status: Needs review » Needs work

Left a comment in the MR.

claudiu.cristea’s picture

Status: Needs work » Needs review

Fixes since https://www.drupal.org/project/drupal/issues/2412669#mr840-note33162

  • Renamed the book backend chained cache service to make it clear what's about.
  • Renamed the book manager internal cache service properties to match the service names.
  • The deprecated drupal_static_reset() should limit the cache invalidation only to memory cache.

I hope this clarifies the concerns from MR. Setting back to NR.

andypost’s picture

added 3 nits to review

claudiu.cristea’s picture

@andypost, fixed 2 of out-of-scope. For the last I've answered in MR.

daffie’s picture

Status: Needs review » Needs work

The MR looks good.

claudiu.cristea’s picture

Status: Needs work » Needs review

@daffie, I've answered your concerns in MR. Setting back to NR. I can provide a commit for last point, related to data provider, if you think we can ignore the small overhead that change would bring.

daffie’s picture

Status: Needs review » Needs work

@claudiu.cristea Thank you for your explanations! If you add the dataProvider method, then the issue is RTBC for me.

claudiu.cristea’s picture

Status: Needs work » Needs review

Converted the test to use data provider.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

All code changes look good to me.
The CR for this issue is [#3039439].
The drupal static cache is no longer used in the BookManager.
For me it is RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Had to double check which memory backend was the correct one to use (the patch is using the right one), so opened #3223546: MemoryCache should not extend MemoryBackend.

Also #3223547: Evaluate BookManager caching to re-evaluate the caching strategy, I think it's fair enough to do a straight conversion here and that evaluation later though.

Looks all fine otherwise, but needs a re-roll.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Rerolled.

  • catch committed e8ff8ea on 9.3.x
    Issue #2412669 by claudiu.cristea, Julfabre, sidharrell, catch, daffie,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed e8ff8ea and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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

quietone’s picture

Publish change record.