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

Implemented: Support for static cache handler system #21

Merged
merged 1 commit into from Oct 7, 2014

Conversation

Projects
None yet
2 participants
@brookinsconsulting
Copy link
Contributor

brookinsconsulting commented Sep 24, 2014

Implemented: Support for static cache handler system. Added StaticCache enabled check before performing executeActions method to mirror kernel implementation

Hello,

First, Thank you again for merging our last static cache related pull request! You continue to make working with your extension so great!

The problem

We recently implemented static cache support for a site and found that content imported with sqliimport did not get static cache created when clearing the view cache via the 'sqliimport_cleanup' cronjob.

After some additional research we found that static cache was not being created because when 'sqliimport_cleanup' cronjob runs to clear the view cache and run the 'eZStaticCache::executeActions()' method we realized we were actually using a StaticCacheHandler and not the eZStaticCache class at all so calling 'eZStaticCache::executeActions()' method due to implementation specific internals of this method would not trigger the storage of delay static cache requests.

See the data stored in our CustomStaticCache::$actionList is not available to the eZStaticCache::$actionList class variable because it is a private variable of a different class, so our previous pull request is not quite enough to properly cover the most common static cache use cases like the use of a StaticCacheHandler.

The solution

The solution to the above problem was to replace the line of code used by the 'sqliimport_cleanup' cronjob part to call 'eZStaticCache::executeActions()' method with '$staticCacheHandlerClassName = eZINI::instance( 'site.ini' )->variable( 'ContentSettings', 'StaticCacheHandler' ); $staticCacheHandlerClassName::executeActions();'.

Which executes the code to store the handler class's delayed requests to update static cache. Without this change the requests to delay static cache updates are never stored and thus never able to performed. For us this was a big negative feature creating a gaping hole in our otherwise perfect static cache setup.

Closing

Please review this pull request.

This change makes it possible for us to continue to sqliimport extension and StaticCacheHandler setting and we would really appreciate it if we did not have to maintain a patch to this extension in a fork.

Please let us know what you think.

Thank you for your continued support!

Cheers,
Brookins Consulting

@brookinsconsulting

This comment has been minimized.

Copy link
Contributor Author

brookinsconsulting commented Oct 5, 2014

Hello,

We have improved this pull request to more closely mirror the implementation provided in the kernel (uses at least two inconsistent implementations). We think this improvement is the polish this pull request needed. This change should make this pull request ready to merge.

Please let us know what you think.

Thank you for your continued support.

Cheers,
Brookins Consulting

'iniVariable' => 'StaticCacheHandler' );
$options = new ezpExtensionOptions( $optionArray );

This comment has been minimized.

@lolautruche

lolautruche Oct 6, 2014

Owner

Extra empty line

$options = new ezpExtensionOptions( $optionArray );
$staticCacheHandler = eZExtension::getHandlerClass( $options );

This comment has been minimized.

@lolautruche

lolautruche Oct 6, 2014

Owner

Extra empty line

@lolautruche

This comment has been minimized.

Copy link
Owner

lolautruche commented Oct 6, 2014

Hi @brookinsconsulting

Thanks. Can you please remove the 2 extra empty lines and squash your commits ?

Implemented: Support for static cache handler system. Added StaticCac…
…he enabled check before performing executeActions method to mirror kernel implementation

@brookinsconsulting brookinsconsulting force-pushed the brookinsconsulting:static-cache-class-override-support branch from cc26d17 to 5f87aa1 Oct 6, 2014

@brookinsconsulting

This comment has been minimized.

Copy link
Contributor Author

brookinsconsulting commented Oct 6, 2014

Hello @lolautruche

Thank you for your feedback and review!

We have removed the extra lines, squashed the commits and pushed up our branch changes for final review and merging.

We think we did this correctly. Please let us know if we did anything wrong or need to do anything more. This is our first time squashing commits :)

Thank you again for your continued support!

Cheers,
Brookins Consulting

lolautruche added a commit that referenced this pull request Oct 7, 2014

Merge pull request #21 from brookinsconsulting/static-cache-class-ove…
…rride-support

Implemented: Support for static cache handler system

@lolautruche lolautruche merged commit fb2a293 into lolautruche:master Oct 7, 2014

@lolautruche

This comment has been minimized.

Copy link
Owner

lolautruche commented Oct 7, 2014

Thanks!

@brookinsconsulting brookinsconsulting deleted the brookinsconsulting:static-cache-class-override-support branch Oct 7, 2014

@brookinsconsulting

This comment has been minimized.

Copy link
Contributor Author

brookinsconsulting commented Oct 7, 2014

Hello @lolautruche

No ... Thank You! We really appreciate your support on this support issue.

We can now remove our class override and use the stock extension without custom modifications!

Thank you again so very very much! Have a great week, you are the best!

If you have a free moment could you vote in support for this very same feature in the default eZ Publish rssimport cronjob part? ezsystems/ezpublish-legacy#1086

Thanking you in advance for your continued support!

Cheers,
Brookins Consulting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment