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

Ss4 upgrade #79

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Conversation

priyashantha
Copy link

Related to the issue #69

@undefinedplayer
Copy link

@tractorcow It'll be great if someone can review this. Appreciate.

@mkrauser
Copy link

mkrauser commented Oct 8, 2018

Hey @priyashantha,

I've also worked on this one and I've got a few suggestions:

  • You are still using super-globals ($_GET, $_POST, $_SERVER) but you replace this using the HTTPRequest-Object
  • I would separate the middleware and the DynamicCache-Class. There might be use-cases where the user wants to use DynamicCache with his own middleware. Also it seems logical.
  • I've been thinking about making the generation of the cache-keys customizeable. There are webserver-plugins which are serving content directly from in-memory-caches (memcached and redis). If we make the keys customizable, we would allow a setup where the content is server directly from the memory, without even touching php). This would be super-super fast. Of course this is not suitable for every installation.

You can take a look at my ss4-branch for the first two points: https://github.com/mkrauser/silverstripe-dynamiccache/tree/ss4
Maybe you can re-use some of my work.

Thanks you for your effort!

@tractorcow
Copy link
Owner

@mkrauser would you like me to just merge your version as the ss4 version? I'm of a mind to let you two decide since you probably have more context over the current upgrade than I do. :)

@priyashantha
Copy link
Author

Thanks @mkrauser for your comments on my update. I'll have a look at them. But I couldn't have a chance to check your fork yet.

So @tractorcow Please merge that if there is no issue there.

@tractorcow tractorcow mentioned this pull request Dec 5, 2018
@tractorcow
Copy link
Owner

I've created #81 as a comparison.

What version do people prefer I merge?

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.

4 participants