Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(dev/core#316) Fix crash on Memcache systems when session key involves whitespace #12653

Merged
merged 8 commits into from
Aug 14, 2018

Conversation

totten
Copy link
Member

@totten totten commented Aug 13, 2018

Overview

As reported in dev/core#316, there's an issue with Memcache compatibility - keys generated by Cache::cleanKey() permit whitespace, which Memcache does not allow.

This patch addresses the bug and also micro-optimizes some cache logic. It is a backport of #12640.

Before

After

  • Cache-keys used for session-management should no longer produce exceptions, even if they include spaces.
  • When reading from the cache, cleanKey() is called once.

xurizaemon and others added 8 commits August 13, 2018 14:16
Since this method is performance related and called frequently,
no need to recalculate results from preg_replace_callback() with
the same inputs.
This is replaces 95b6567.  The intent is to
have a delimiter every ten chars so that it's easy to read/confirm the
length of the string.

The delimiter was space, but this became a longer encoded char (`-20`)
and threw off the numbers. Switching to dash just gives a different encoded char.
To get the counts right, it needs ot be a pass-through char... like underscore.
@civibot
Copy link

civibot bot commented Aug 13, 2018

(Standard links)

@totten totten changed the title 5.4 spaces (dev/core#316) Fix crash on Memcache systems when session key involves whitespace Aug 13, 2018
@eileenmcnaughton
Copy link
Contributor

This has been approved for the rc so OK to merge here.

@eileenmcnaughton eileenmcnaughton merged commit e9e5510 into civicrm:5.4 Aug 14, 2018
@totten totten deleted the 5.4-spaces branch August 17, 2018 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants