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

3.7 Improve Cache Controller #12312

Merged
merged 2 commits into from
Jan 27, 2017
Merged

3.7 Improve Cache Controller #12312

merged 2 commits into from
Jan 27, 2017

Conversation

csthomas
Copy link
Contributor

@csthomas csthomas commented Oct 4, 2016

Pull Request for Issue #12133 and other optimizations.

Summary of Changes

  • Deprecate get and store methods in JCacheController, subclasses should have own methods.
  • Deprecate start and end methods in JCacheControllerOutput - joomla does not use it
  • Deprecate call method in JCacheControllerCallback
  • Remove setLifeTime, setCaching from JCacheController because __call() method can call it directly on JCache instance.
  • Replace operator == to === for locked and locklooped variables.
  • Do not store if lock can not be locked:
if ($locktest->locked === false && $locktest->locklooped === true)
  • Add get and store method to JCacheControllerOutput which is the same as deprecated method get and store in JCacheController.
  • In subclasses of JCacheController I did optimization on get methods.
    • move unlock() method before unserialize(...)
    • Generally unlock cache as fast as we can
  • Do not use ternary operator on variables with a lots of data, see http://fabien.potencier.org/the-php-ternary-operator-fast-or-not.html

Testing Instructions

Joomla should work as before.
Code review.

Documentation Changes Required

None.

@csthomas
Copy link
Contributor Author

Can I ask you @mbabker for a review?

Copy link
Contributor

@mbabker mbabker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, one PHPCS issue.

{
$data['output'] = JCache::setWorkarounds($output, $coptions);
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHPCS issue.

@csthomas
Copy link
Contributor Author

Thanks Michael.

Now I have to wait for 2 successful tests:)

@ghost
Copy link

ghost commented Jan 11, 2017

@csthomas Enable Cache and test if J works as before?

@csthomas
Copy link
Contributor Author

Yes, be sure that cache works before patch

@ghost
Copy link

ghost commented Jan 12, 2017

I have tested this item ✅ successfully on 87f42d5

Tested on Articles, Blog, List, Contact, Feed, Search.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12312.

@JoshJourney
Copy link

I have tested this item ✅ successfully on 87f42d5

Logged in, logged out, created article, viewed list, viewed article, changed site settings on frontend, ect. Seems to work great on every view I tested. Successfully tested this PR.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12312.

@jeckodevelopment
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12312.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 27, 2017
@jeckodevelopment jeckodevelopment added this to the Joomla 3.7.0 milestone Jan 27, 2017
@rdeutz rdeutz merged commit a3194fa into joomla:staging Jan 27, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 27, 2017
@csthomas csthomas deleted the cache-controller branch January 27, 2017 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants