Problem/Motivation

In refreshVariables() we have \Drupal::service('cache_tags.invalidator.checksum')->reset();. If you are using a different cache reset service you are SOL.

Proposed resolution

Reset all the invalidators. Tricky problem because CacheTagsInvalidator has the $invalidators protected. Add a getter? Add a resetCache method warning for test only? Or no need for warning as it's not harmful? I do not see clearly.

CommentFileSizeAuthor
#13 cache-tags-reset-2433595-13.patch1.39 KBBerdir
#2 2433595_2.patch3.18 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Given that CacheTagsChecksumInterface::reset is marked as test only, I think it would be totally valid to move this method onto CacheTagsInvalidatorInterface intstead
and call it from the collector.

chx’s picture

chx CreditAttribution: chx commented
Status: Active » Needs review
FileSize
2433595_2.patch3.18 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Looks fine for me!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

I guess we need something like this. I tried to avoid it, because it doesn't make sense for almost all cache tag invalidators, only those that persist the invalidations and the whole reset() thing is a hack for running tests.

Note that this is a small API change for existing implementations.

Related: #2250033: Garbage collection for cache tag invalidations

chx’s picture

chx CreditAttribution: chx commented

> Note that this is a small API change for existing implementations.

If your invalidator implements only one of the interfaces -- but most implement both. Also, how many invalidator backends are out there??

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

No, only a single one implements the checksum interface.

I've written 4 different implementations already :) None of them needs reset.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review

Can we avoid the API change? If we thought all backends would need it then fine, but since some/most won't then it's not great to require the API change for no benefit.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

@chx, what exactly is your use case for needing to be called?

Are you providing a checksum implementation and are implementing that interface? Or is it a standalone invalidator that has a static cache that needs to be reset?

If the first, then we could check for that interface in that loop and if it is the second, then we could move reset() to a separate interface and check for that?

chx’s picture

chx CreditAttribution: chx commented

Most often MongoDB very closely mirrors the SQL drivers. Often they are written by copying the SQL class and converting/removing the SQL specific parts. Sometimes (like this one) I do not even understand what's up. As with the interaction of the entity storage drivers vs constructing entities have shown deviating from the core path is problematic and so is understanding what's up. I try to file documentation patches to make it all easier once I understood them.

So, to answer @Berdir's question, if core has one then mine has too. I do not see the word static in either class so I am totally baffled to be honest.

chx’s picture

chx CreditAttribution: chx commented

http://cgit.drupalcode.org/mongodb/tree/src/CacheTagsChecksumMongodb.php...
http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Cache/Databa...
both have:

  public function reset() {
    $this->tagCache = array();
    $this->invalidatedTags = array();
  }

is one of these the "static cache" ? Which one? Why do we call it static?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Sorry, static cache was confusing. Yes, I meant those properties. they used to be drupal_static() and still work the same, that's why I used that term.

Ok, that means you have a checksum service, then adding an instanceof check for that interface should work fine for you?

chx’s picture

chx CreditAttribution: chx commented

Absolutely, anything works for me: as long as it works for core and it is not hardwired.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
cache-tags-reset-2433595-13.patch1.39 KB

This is what I had in mind.

The new method is not on an interface, but it's used only by tests, so I'm not sure we have to worry about that. If yes, then we could implement a standalone interface for that.

chx’s picture

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

So I am fine with this and #13 has nothing to do with what I coded so I am putting it in the ready queue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

No API change and no disruption and Mongo can work with this. Committed b6ee00e and pushed to 8.0.x. Thanks!

  • alexpott committed b6ee00e on 8.0.x 
    Issue #2433595 by chx, Berdir: WebTestBase::refreshVariables only resets...

Status: Fixed » Closed (fixed)

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