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/cloud-native#3) SerializeCache - Remove unused, incomplete cache-driver #14717

Merged
merged 1 commit into from
Jul 3, 2019

Conversation

totten
Copy link
Member

@totten totten commented Jul 3, 2019

Overview

The class CRM_Utils_Cache_SerializeCache provides a cache-driver for
storing cache records in the filesystem (under CIVICRM_TEMPLATE_COMPILEDIR,
using PHP serialize() format). Why remove it?

  1. As we work through cleanup in file management (e.g. Lookup path-variable for the private filepath #12843,
    dev/cloud-native#3), having an unnecessary reference to
    CIVICRM_TEMPLATE_COMPILEDIR makes it harder to reason about the system.

  2. The class is not used. I'm pretty sure it was added speculatively
    (i.e. with an aim to try using it some day), but that never came to
    pass. Grepping universe, I cannot find any usages or references to
    SerializeCache.

  3. The implementation is incomplete -- parameters like get(..., $default)
    and set(..., $ttl) will generate exceptions if used.

Before

  • Driver exists and is not used.

After

  • Driver does not exist.

…-driver

The class `CRM_Utils_Cache_SerializeCache` provides a cache-driver for
storing cache records in the filesystem (under `CIVICRM_TEMPLATE_COMPILEDIR`,
using PHP `serialize()` format). Why remove it?

1.  As we work through cleanup in file management (e.g.  civicrm#12843,
   dev/cloud-native#3), having an unnecessary reference to
   CIVICRM_TEMPLATE_COMPILEDIR makes it harder to reason about the system.

2. The class is not used. I'm pretty sure it was added speculatively
   (i.e.  with an aim to try using it some day), but that never came to
   pass.  Grepping `universe`, I cannot find any usages or references to
   `SerializeCache`.

3. The implementation is incomplete -- parameters like `get(..., $default)`
   and `set(..., $ttl)` will generate exceptions if used.
@civibot
Copy link

civibot bot commented Jul 3, 2019

(Standard links)

@civibot civibot bot added the master label Jul 3, 2019
@eileenmcnaughton
Copy link
Contributor

@totten test failure looks like server needs a kick

@eileenmcnaughton
Copy link
Contributor

I agree with your analysis - I did a couple of further checks

  1. checked that no functional commits had taken place on the file since it was brought over from svn (it did get some changes as part of various cleanups & stdisations but none that suggest it was targetted)
  2. searched for a 'Serialize' (case sensitive) in case it was being concatenated in the core code base
  3. checked the references to cache options in civicrm.settings.template.php

I feel comfortable this can be removed & we can stop 'fixing it' to meet standards

@totten
Copy link
Member Author

totten commented Jul 3, 2019

jenkins, test this please

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton eileenmcnaughton merged commit 950468d into civicrm:master Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants