Massive review and cleanup of JCache

[mbabker]

Unfortunately this PR does a lot. It started just trying to clean up some of the tests but then several bits of dead code and bug fixes were needed to, so...

Summary of Changes

This PR makes the following changes:

• General changes
  • Creates an abstract TestCaseCache for all JCacheStorage handler tests and rewrites the test classes for those objects to use this test case (results in more consistent testing of all handlers when the environment supports it)
  • Standardizes doc blocks on all JCache classes for full consistency and removes some outdated or unneeded comments
  • Uses late static bindings where practical (enables better class inheritance)
• Bug fixes
  • JCacheStorageMemcache::store() was manipulating the handler's lifetime setting after being instantiated
  • JCacheStorageRedis::store() was manipulating the handler's lifetime setting after being instantiated, had some dead code (copy/paste from Memcache(d) not needed here), and used a hardcoded lifetime instead of the handler's configured lifetime

Testing Instructions

[brianteeman]

(a bit above my skill set but sounds like this might address some existing issues here https://issues.joomla.org/tracker/joomla-cms/?state=open&sort=issue&direction=desc&category=cache&stools-active=1 )

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9622.}

[mbabker]

#9382 is included in here. #8707 I've added the fix for (when JCache creates a handler the lifetime is already injected in as a constructor parameter and JCacheStorage handles conversion of minutes to seconds, as noted there this line just screws things up). Ironically #6796 also makes that same change based on #2539.

[brianteeman]

Thanks for checking can you update those issues as needed

On 27 March 2016 at 19:12, Michael Babker notifications@github.com wrote:

#9382 #9382 is included in
here. #8707 #8707 I've added
the fix for (when JCache creates a handler the lifetime is already
injected in as a constructor parameter and JCacheStorage handles
conversion of minutes to seconds, as noted there this line just screws
things up). Ironically #6796
#6796 also makes that same
change based on #2539 #2539.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9622 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

[brianteeman]

@mbabker I have updated the other issues. Thanks for reviewing them

[andrepereiradasilva]

I have tested this item ✅ successfully on 9228a86

Wow that's really a massive review!

Did a code review (except the unit tests) and all seems fine.

Installed the patch turned on File cache and all seems fine.

Didn't test all the other cache handlers, but by looking at code changes, it seems fine and also corrects some bugs.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9622.}

[alikon]

i've tested successfully using only the redis handler

[Kubik-Rubik]

@alikon Please use the Joomla! issue tracker to enter your test result. Thanks!

[alikon]

I have tested this item ✅ successfully on 9228a86

Tested on Redis cache handler

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9622.}

[brianteeman]

RTC thanks

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9622.}

[csthomas]

This line is incorrect!
$server is not defined, $memcachetest return false, and below throw exception.

This is reason why travis on PR #11576 failed.
https://travis-ci.org/joomla/joomla-cms/jobs/151946004

[mbabker]

Oh. Oops.

On Saturday, August 13, 2016, Tomasz Narloch notifications@github.com
wrote:

In libraries/joomla/cache/storage/memcache.php
#9622 (comment):

•   $memcachetest = @self::$_db->connect($server['host'], $server['port']);
  

•   $memcachetest = @static::$_db->connect($server['host'], $server['port']);
  

This line is incorrect!
$server is not defined, $memcachetest return false, and below throw
exception.

This is reason why travis on PR #11576
#11576 failed.
https://travis-ci.org/joomla/joomla-cms/jobs/151946004

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/joomla/joomla-cms/pull/9622/files/9228a8674c558e12d6af931e2b8785effa1bbbb8#r74692172,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoYsotSVZ4Tb6QFw_5WMQsQW3pRhqks5qfi8sgaJpZM4H5gT6
.

[rdeutz]

Is this fixed somewhere or do we have to fix it, just curious.

[csthomas]

It is fixed at #11565.

[rdeutz]

@csthomas Thanks!