At the moment if you implement Redis for your page/block cache, you'll end up delivering some stale content to users. This is because of the logic Drupal uses for cache_clear_all():

http://api.drupal.org/api/drupal/includes%21cache.inc/function/cache_cle...

When a new node is submitted, this function is called with no arguments, resulting in:

  cache_clear_all(NULL, 'cache_page');

Which turns into:

  _cache_get_object($bin)->clear($cid, $wildcard);

Which results in the clear() method bailing out:

    // Redis handles for us cache key expiration.
    if (!isset($cid)) {
      return;
    }

Since isset() will return FALSE if the value of $cid is NULL.

Further, the logic for what to do when the $cid is not an asterisk and the wildcard is not set needs to be adjusted.

CommentFileSizeAuthor
#1 cache-clear-all-1555210-1.patch1.08 KBjoshk

Comments

joshk’s picture

joshk commented
StatusFileSize
new cache-clear-all-1555210-1.patch1.08 KB

Here's a patch.

joshk’s picture

joshk commented
Status: Active » Needs review
mstef’s picture

mstef commented

Works for me

pounard’s picture

pounard
Primary language French
commented

Thank you very much, I'll review and commit this.

pounard’s picture

pounard
Primary language French
commented

Fixed in 2.x branch, I will not fix the 1.x which is not supported anymore: http://drupalcode.org/project/redis.git/commit/853c8fe

Thanks joshk for the patch (I slightly modified it and added some documentation), report and debug, and thanks mikestefff for review and test.

I will do a new proper release in a few hours, I have some other fixes to do.

pounard’s picture

pounard
Primary language French
commented
Status: Needs review » Fixed
joshk’s picture

joshk commented

Any update on a release including this patch? I would love to tell some folks to grab the new release.

omega8cc’s picture

omega8cc commented

Sorry if this is a stupid question, but why are you turning away the standard order in comparison, so it is

else if ('*' !== $cid && $wildcard) {

and not

else if ($cid !== '*' && $wildcard) {

Plus, why else if and not elseif? It is a PHP code, not JS :)

pounard’s picture

pounard
Primary language French
commented

About the "reverse" if, always doing that for two reasons:

  1. It highlights the constant you are compairing, and not the variable, which is the most important thing to put into light when you are doing N comparaisons of the same variable, this is pure syntactic sugar.
  2. Most importantly, if you do a typo error such as if ($variable = CONSTANT), PHP will interpret the = as an allocation and will always return the CONSTANT value cast as boolean, where it should fail. If you write the same typo the other way arround, such as: if (CONSTANT = $variable) PHP will do a fatal error instead, because you cannot allocate a constant value: the error will be spotted at first run and you won't commit erroeneous code :)

About the else if, it's valid and legal in PHP as well, so it's only a matter of taste. Personally I prefer else if to elseif, in my mind it's natural english therefore it's more readable for a human mind (and the computer mind doesn't care and interpret both the same way).

I'd say in both cases, both syntaxes are valid, it's only a matter of taste. In both case, I find the way I wrote them more naturally readable for the human mind (but other people may not, as I said, it's not definitive, it's about taste and habbits). In the case of the constant comparison with the constant on the left, it also protects you against a very recurrent typo error, which is, in that particular case, also a security and consistency matter.

I'm also reversing the if for other language constants, for value constants, and function calls, such as, for example:

  • if (null === $my_value)
  • if (false === $some_variable)
  • if (2 === $result)
  • if (request_path() === 'node')

Also always forcing the === for most of those, values should always be typed right, a value with the wrong type (for example when you fetched an integer from the database but let live as a string, or fetched a value from unsafe GET parameters but you didn't convert it) is a design and logic error, because it's unsafe (it proves here that you didn't escaped or parsed the value).

pounard’s picture

pounard
Primary language French
commented

@joshk I will try to take some time to do the release during the weekend then.

pounard’s picture

pounard
Primary language French
commented

Ok I just released the 7.x-2.0-alpha9 including this fix only. It will be available for download in a few minutes.

joshk’s picture

joshk commented

You rock! Thanks!

omega8cc’s picture

omega8cc commented

Wow! I didn't expect such detailed response, thank you!

Status: Fixed » Closed (fixed)

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

  • Commit 853c8fe on 7.x-2.x, 7.x-2.x-sharding, 7.x-2.x-path by Pierre.R: 
    #1555210 - authored by joshk - cache_clear_all() does nothing with...