Skip to content
Snippets Groups Projects

Resolve #2412669 "Remove drupal_static from BookManager"

Resolve #2412669 "Remove drupal_static from BookManager"
issue/drupal-2412669:2412669-bookmanager into 9.3.x
3 open threads
Open
Claudiu Cristearequested to merge
issue/drupal-2412669:2412669-bookmanager into 9.3.x
3 open threads

Closes #2412669

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Ted Bowman
  • Ted Bowman
  • Ted Bowman
  • Claudiu Cristea added 1 commit

    added 1 commit

    • f5557bd6 - Review remarks. Simplify tests. Better deprecation message

    Compare with previous version

  • Claudiu Cristea added 1 commit

    added 1 commit

    Compare with previous version

  • Claudiu Cristea added 76 commits

    added 76 commits

    Compare with previous version

    Toggle commit list
  • Claudiu Cristea resolved all threads

    resolved all threads

  • Sascha Grossenbacher
    Sascha Grossenbacher @berdir started a thread on an old version of the diff
    • core/modules/book/book.services.yml
    30 30 - [setContainer, ['@service_container']]
    31 31 tags:
    32 32 - { name: cache.context}
    33
    34 33 book.uninstall_validator:
    35 34 class: Drupal\book\BookUninstallValidator
    36 35 tags:
    37 36 - { name: module_install.uninstall_validator }
    38 37 arguments: ['@book.outline_storage', '@entity_type.manager', '@string_translation']
    39 38 lazy: true
    39 book.memory_cache:
    40 class: Drupal\Core\Cache\MemoryCache\MemoryCache
    41 book.cache:
    • Sascha Grossenbacher
      Sascha Grossenbacher @berdir
      Developer

      I don't understand why this introduces a new persistent cache bin. It used data before. We deliberately reduced the amount of bins we have in D8 instead of letting each module defines its own. Each bin has it's own semantics, e.g. data is one that can have a ton of different cids, so people might have configured a different backend for that than e.g. default/boostrap which is used often but should require less memory/data. What's wrong with using data?

      I skimmed through the comments but missed why a separate memory cache was introduced here.

      It makes sense to me to have that for entities where clearing caches is fairly common and for now has some uses outside of tests but I don't see that we'd store a lot of data for book in memory or that clearing those caches is common. In fact, I see not a single call to that, not even from tests.

      An issue summary update would be helpful to catch up with the changes.

    • Claudiu Cristea
      Claudiu Cristea @claudiucristea
      Author

      @berdir

      I don't understand why this introduces a new persistent cache bin

      No, it doesn't introduce any new persistent cache bin. It uses BackendChain, which allows to define several layers of cache. In this case we use memory cache as first layer and cache.data persistent cache as the 2nd layer. There's a "tradition" in Drupal to do this:

      function getFoo() {
  static $foo; // may be drupal_static()
  if (!isset($foo)) {
    if ($cache = \Drupal::service('cache.data')->get('cid:foo')) {
      $foo = $cache->data;
    }
    else {
      $foo = ...heavy work
      \Drupal::service('cache.data')->set('cid:foo', $foo);
    }
  }
  return $foo;
}

      BackendChain is doing the same but in a more elegant way. There are a lot of advantages, but one of them is that we get rid of the classic protected property static cache which, as we all know, is a pain when it comes to invalidation. Right now, core has a single usage of BackendChain, see the cache.jsonapi_resource_types service.

      What's wrong with using data?

      Nothing. We're still using cache.data.

      (...) but I don't see that we'd store a lot of data for book in memory or that clearing those caches is common

      @berdir, I didn't question the current approach and why they're using static+persistent cache mechanism. I guess there are a lot of reasons, most probable building the book tree is expensive. I've only tried to do the conversion. As the memory cache was done via drupal_static(), that means it has to be a mechanism to invalidate from outside. And I really wanted to avoid an BookManager::resetCache(), which looks like a workaround, compared to a memory cache service, which benefits from cache tags.

    • Sascha Grossenbacher
      Sascha Grossenbacher @berdir
      Developer

      Ah, I missed that this is a cache chain, sorry. Yeah, understand how those work.

      I commented on the service definition, but actually didn't really look at that, I just had a fairly quick look at the BookManager changes. It reflects the existing scenario of sometimes only using static/memory caching and sometimes both, but the variable names and the fact that we inject both didn't make it clear to me that one of them is in act a chain and how that works together (missed the parameter description. as I said, quick look :)). Also, the unit test even injects the same instance for both.

      The memory bin isn't tagged, so it's only getting the cache tag invalidation through the chain, more confusion ;) I initially considered to suggest to make that private to reflect that, but that was before I remembered that it is injected into BookManager too.

      FWIW, none of these caches use cache tags, and there is no existing cache invalidation, neither in core nor contrib (http://grep.xnddx.ru/search?text=drupal_static_reset(%27Drupal\book), nothing explicit outside of the generic full resets for tests and stuff anyway.

      No specific suggestions (except maybe renaming the variables/properties), just thinking/writing out loud...

    • Claudiu Cristea changed this line in version 8 of the diff

      changed this line in version 8 of the diff

    • Please register or sign in to reply
  • Claudiu Cristea added 37 commits

    added 37 commits

    Compare with previous version

    Toggle commit list
  • Claudiu Cristea added 1 commit

    added 1 commit

    • 9bc18828 - Book module might be disabled.

    Compare with previous version

  • Claudiu Cristea added 13 commits

    added 13 commits

    • 9bc18828...dc9a09e8 - 2 commits from branch project:9.3.x
    • 6af16ce0 - Patch #2412669-58
    • b589ccd1 - Fix tests.
    • f3cce964 - Fix deprecation messages
    • d60f1786 - Coding standards
    • 6fffd72c - Review remarks. Simplify tests. Better deprecation message
    • 5e8827d8 - Small docs fix
    • 9d757e8b - Consolidate deprecation message pattern in drupal_static_reset()
    • ebb9e0dd - Book module might be disabled.
    • bd50a89f - Avoid confusion: rename book.cache book.backend_chained_cache
    • 3ce17718 - Make it clear why we use the same mock in unit test
    • 9d07f1c7 - Limit drupal_static_reset() to memory cache invalidation

    Compare with previous version

    Toggle commit list
  • Claudiu Cristea added 1 commit

    added 1 commit

    • f7e3a1f9 - Rename cache services internal properties to match the service names

    Compare with previous version

  • Claudiu Cristea added 1 commit

    added 1 commit

    Compare with previous version

  • Andrey Postnikov
  • Andrey Postnikov
  • Andrey Postnikov
  • Claudiu Cristea added 1 commit

    added 1 commit

    Compare with previous version

  • daffie
  • daffie
  • daffie
  • daffie
    daffie @daffie started a thread on an old version of the diff
    • core/modules/book/tests/src/Kernel/BookManagerDeprecationTest.php 0 → 100644
    8 /**
    9 * @coversDefaultClass \Drupal\book\BookManager
    10 * @group legacy
    11 */
    12 class BookManagerDeprecationTest extends KernelTestBase {
    13
    14 /**
    15 * {@inheritdoc}
    16 */
    17 protected static $modules = ['book'];
    18
    19 /**
    20 * @see drupal_static_reset()
    21 */
    22 public function testDrupalStaticResetDeprecation(): void {
    23 foreach (['bookSubtreeData', 'bookTreeAllData', 'doBookTreeBuild'] as $method) {
  • Claudiu Cristea added 1 commit

    added 1 commit

    • 35965d9d - Move to a data provider approach

    Compare with previous version

  • catch @catch started a thread on the diff
    • core/modules/book/src/BookManager.php
    102 117 * The language manager.
    103 118 * @param \Drupal\Core\Entity\EntityRepositoryInterface|null $entity_repository
    104 119 * The entity repository service.
    120 * @param \Drupal\Core\Cache\CacheBackendInterface $backend_chained_cache
    • C
      catch @catch
      Maintainer

      I was confused why we're injected both services when the memory cache is already chained, and it's because in some methods there's only a static cache and in some only a persistent cache.

      I think we need at least a follow-up to look into whether these can be brought into line - either all persistent, or removing the static-only caching altogether if it's not needed, or documenting why we're using one or the other.

    • Please register or sign in to reply
  • Claudiu Cristea added 27 commits

    added 27 commits

    • 35965d9d...e2cbb459 - 12 commits from branch project:9.3.x
    • 7de21bfb - Patch #2412669-58
    • dce03aaa - Fix tests.
    • 3c7019a4 - Fix deprecation messages
    • 826a7447 - Coding standards
    • 32933f7f - Review remarks. Simplify tests. Better deprecation message
    • 32ebbd55 - Small docs fix
    • c3c361de - Consolidate deprecation message pattern in drupal_static_reset()
    • 4256e799 - Book module might be disabled.
    • ff7c515c - Avoid confusion: rename book.cache book.backend_chained_cache
    • 6b5f6d06 - Make it clear why we use the same mock in unit test
    • 2009c826 - Limit drupal_static_reset() to memory cache invalidation
    • 769f1f47 - Rename cache services internal properties to match the service names
    • 266175fb - Adapt the deprecation test
    • 5189eac0 - Undo out-of-scope changes
    • fb938a4d - Move to a data provider approach

    Compare with previous version

    Toggle commit list
    • Please register or sign in to reply