[#9622] - Massive review and cleanup of JCache
- Fixed in Code Base
- 12 Apr 2016
- Medium
- Build: staging
- # 9622
- Diff
- mbabker:Cache-Test-Cases
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
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
TestCaseCachefor allJCacheStoragehandler 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
JCacheclasses for full consistency and removes some outdated or unneeded comments - Uses late static bindings where practical (enables better class inheritance)
- Creates an abstract
- 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
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
?
|
||
| Category | ⇒ | Cache Libraries Unit Tests |
| Labels | |||
| Labels | |||
#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.
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/
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.
i've tested successfully using only the redis handler
| Milestone |
Added: |
||
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.
| Status | Pending | ⇒ | Ready to Commit |
| Labels | |||
RTC thanks
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9622.
| Labels |
Added:
?
|
||
rdeutz
- | Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-04-12 20:02:31 |
| Closed_By | ⇒ | rdeutz |
| Milestone |
Removed: |
||
| Milestone |
Added: |
||
| Milestone |
Added: |
||
| Milestone |
Removed: |
||
| Labels |
Removed:
?
|
||
(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.