Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|
Issue fork drupal-2412669
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:
Comments
Comment #1
sidharrell CreditAttribution: sidharrell commentedPlease review with some kindness. My first time actually changing code on an issue. Thanks :)
Comment #2
ParisLiakos CreditAttribution: ParisLiakos commentedthanks 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
Comment #3
meramo CreditAttribution: meramo commentedChanging status, obviously needs more work
Comment #4
Julfabre CreditAttribution: Julfabre commentedI'm working on it for drupal dev day's
Comment #5
Julfabre CreditAttribution: Julfabre commentedWe refactor for 3 differents properties.
Comment #6
maartendeblock CreditAttribution: maartendeblock at EntityOne commentedTested, the book functionality still works fine.
Comment #7
alexpottThis 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
Comment #14
claudiu.cristeaI guess this can be un-postponed.
Comment #15
claudiu.cristeaComment #16
claudiu.cristeaComment #17
claudiu.cristeaFixed the IS. Rerolled an performed only some cosmetic changes. Let's see the bots.
Comment #18
BerdirSeems pretty straight-forward, only reason this wasn't committed 3 years ago is that it was too late during the beta phase.
Comment #19
catchParisLiakos' 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?
Comment #20
claudiu.cristeaOK, namespaced the cache, even, IMO, the cid prefix was safe enough.
Comment #22
claudiu.cristeaOk.
Comment #23
claudiu.cristeaI'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 thebook-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.
Comment #24
Mile23Just some almost-nits:
Maybe document the literal keys and their meaning in the $this->cache docblock?
Maybe rename to $data_cache or something so that the names are distinct?
Comment #26
claudiu.cristeaFixed #24.
Now an aspect that came after I learn from deprecating lots of
drupal_static()
& Co.What if a 3rd party code did this?
Until we deprecate, that is a legit usage of our API because
drupal_static()
anddrupal_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 ofdrupal_static_reset()
for this specific cases. I've added a publicresetCache()
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 theswitch () {...}
statement (to be extended).I've added also tests and a CR to outline this deprecation: https://www.drupal.org/node/3039439.
Comment #27
kim.pepperInject?
Inject?
Do we need to reset the cache backend too?
Should this be a kernel test?
Should deprecation tests be in a separate legacy test class?
Comment #28
claudiu.cristeaFixed #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.
Comment #30
catchTwo 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
Comment #31
claudiu.cristea@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.Comment #32
claudiu.cristeaFixing 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.
Comment #33
andypostDoes 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
Comment #34
andypostForum manager also caches some data but use some magic for serialisation
Comment #35
claudiu.cristea@catch, @andypost we should get first a decision in #3047289: Standardize how we implement in-memory caches.
Comment #36
claudiu.cristeaHere 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.
Comment #37
Berdirhttps://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.
Comment #39
claudiu.cristea@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.
Comment #40
claudiu.cristeaSome small improvements.
Comment #41
claudiu.cristeaComment #42
kristiaanvandeneyndeWhy 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.
Comment #43
claudiu.cristea@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.
Comment #44
kristiaanvandeneyndeWhat's the reasoning behind this? I can see multiple modules wanting to use MemoryCache, having a factory would only simplify this.
Comment #45
claudiu.cristea@kristiaanvandeneynde
VS.
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.
Comment #46
catchCurrent entries for memory cache services look like this:
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.
Comment #47
claudiu.cristeaOther minor fixes.
Comment #48
kristiaanvandeneynde@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']).
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 :(
Comment #49
claudiu.cristea@kristiaanvandeneynde, please move this discussion in the proper issue.
Comment #50
kristiaanvandeneynde@claudiu.cristea That's what I just said :D
Comment #51
claudiu.cristea#3061117: Make optional and deprecate the BackendChain constructor parameter was merged. Removing unneeded argument from service.
Comment #52
daffie CreditAttribution: daffie commentedThe patch looks good to me. Only 1 nitpick:
I am missing the "all" in the new version.
Comment #53
claudiu.cristea@daffie that is superfluous. The uniqueness of the CID is ensured without "all". It cannot collide with other CIDs.
Comment #55
claudiu.cristeaOops, new test changes.
Comment #57
claudiu.cristeaD 8.9.x
Comment #58
claudiu.cristeaAs 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.
Comment #60
tedbowI think this needs to happen first on 9.3.x now, correct? If so it needs a re-roll
Comment #62
claudiu.cristeaRerolled, adapted deprecation messages. I moved to a MR
Comment #63
claudiu.cristeaComment #64
tedbowNeeds work for comments in Gitlab
Comment #65
claudiu.cristeaComment #66
tedbow@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 soonComment #67
BerdirLeft a comment in the MR.
Comment #68
claudiu.cristeaFixes since https://www.drupal.org/project/drupal/issues/2412669#mr840-note33162
I hope this clarifies the concerns from MR. Setting back to NR.
Comment #69
andypostadded 3 nits to review
Comment #70
claudiu.cristea@andypost, fixed 2 of out-of-scope. For the last I've answered in MR.
Comment #71
daffie CreditAttribution: daffie commentedThe MR looks good.
Comment #72
claudiu.cristea@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.
Comment #73
daffie CreditAttribution: daffie commented@claudiu.cristea Thank you for your explanations! If you add the dataProvider method, then the issue is RTBC for me.
Comment #74
claudiu.cristeaConverted the test to use data provider.
Comment #75
daffie CreditAttribution: daffie commentedAll 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.
Comment #76
catchHad 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.
Comment #77
claudiu.cristeaRerolled.
Comment #79
catchCommitted e8ff8ea and pushed to 9.3.x. Thanks!
Comment #81
quietone CreditAttribution: quietone at PreviousNext commentedPublish change record.