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

Basic built-in cache #32

Closed
wants to merge 8 commits into from
Closed

Conversation

rudyrigot
Copy link
Contributor

No description provided.

@lsmith77
Copy link
Contributor

this seems to be a very important feature and might be easy to do thanks so this guzzle plugin:
http://guzzle.readthedocs.org/en/latest/plugins/cache-plugin.html

@rudyrigot
Copy link
Contributor Author

This could work, I only think of two problems that I can see and I'd love to get your opinion on:

  • since documents are immutable in prismic.io, we like to base our cache invalidations rather on a LRU approach than on expiration dates (since the API calls don't actually have an expiration date). This is not a big deal though, it'd already be great to have a basic caching mechanism running.
  • since this would be included in the kit, it needs to be agnostic to the rest of the application's architecture, so ideally, keeping the cache entries in memory rather that in files would probably be better. I'm trying to look for a way to use this plug-in in that way, but I can't find one, so I feel like we'd have to build a "MemoryCacheStorage" class that would implement CacheStorageInterface. Am I thinking of this right?

Also, the cache needs to be overridable, but that'd be easy: for instance, offering to take an alternative CachePlugin object as a parameter of the Api class's constructor.

@lsmith77
Copy link
Contributor

I didn't dig into that yet but I see your points. maybe @mtdowling can give us some hints here .. we may also want to upgrade to guzzle 4.0

@stloyd
Copy link

stloyd commented Apr 22, 2014

@lsmith77 You can't use Guzzle 4.0 if you still want to support PHP 5.3.

@rudyrigot
Copy link
Contributor Author

This is relevant, considering PHP 5.3 is very used, and there are quite a bunch of hosting providers our there that don't support 5.4.

So much for Guzzle 4.0, but that doesn't mean we can't do the caching with Guzzle's cache plugin anyway (so this ticket is still relevant).

@mtdowling
Copy link

there are quite a bunch of hosting providers our there that don't support 5.4.

Do you have actual data on this?

@rudyrigot
Copy link
Contributor Author

Not actually, I was only speaking from experience with hosting providers; but the first link about PHP usage (this one) is kinda enough data for this matter here. Since prismic.io is a service in your application, it's great to build up brand new projects, but it's also great to add into existing projects, and may definitely be used as such by some users.

@rudyrigot
Copy link
Contributor Author

Alright, I think I understand better how this needs to be done.
It seems like CacheStorage is a layer that deals with the HTTP stuff (such as expirations if there are some set, any other kind of HTTP header, etc.), and passes the cache key (I guess, the URL + HTTP verb) and the cache data (I guess, the content of the response) on to the next layer, CacheAdapter, which simply has to deal with them as strings.
None of the CacheStorageInterface and CacheAdapterInterface interfaces have implementations to store in memory; this means we'll need to get our hands dirty. And knowing what I stated above, it makes more sense to get our hands dirty implementing CacheAdapterInterface (which simply deals with strings) than CacheStorageInterface.

Looking at the existing implementations of CacheAdapterInterface, I seem to understand there are two ways to do this:

  • either create a class that implements the interface, simply.
  • or using ClosureCacheAdapter, where the 4 functions of the interface are expected as closures passed in the constructor; but no matter how I look at this, I really can't see an upside to the latter. Please tell me I'm wrong if I am, they're both equally feasible.

Alright, these were my thought as I get into it, I'll get started with this now. I'm new to this, so if anyone feels like giving a hand, any help is welcome (first: does what I just stated above make sense? am I headed the right direction?)

Thanks!

@lsmith77
Copy link
Contributor

lsmith77 commented May 6, 2014

haven't gotten a chance to test this yet.
on skimming the code I noticed that the formatting is not PSR-2 compliant:
https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md

not sure if you guys intend to follow this standard or not.

@rudyrigot
Copy link
Contributor Author

Sure, every standard going in the right direction is good!

Do you mean the whole code of the kit, or just the new code from this PR? (If whole kit, I'll open a separate issue) Which violations of PSR-2 did you notice, so it gives me pointers?

It's a rather dense standard, I need to learn it first; but I'll get into it today.

@lsmith77
Copy link
Contributor

lsmith77 commented May 6, 2014

saw a few in this PR didnt check the rest of the code. easiest is just to use https://github.com/fabpot/PHP-CS-Fixer

@rudyrigot
Copy link
Contributor Author

I didn't know this existed, and this is definitely going to be helpful; thanks a lot!

@stloyd
Copy link

stloyd commented May 6, 2014

In overall I would recommend to setup Scrutinizer-CI, it's free for OS & it's "like" Jenkins =)

@rudyrigot
Copy link
Contributor Author

We already use TravisCI, but I feel like Travis is more unit-test oriented, and Scrutinizer is more about code quality (which would make it a bit more like Sonar); am I getting this right?
Is it possible (and safe) to connect both TravisCI and ScrutinizerCI on the same repository?

@lsmith77
Copy link
Contributor

lsmith77 commented May 6, 2014

yes and yes .. scrutinizer is about code quality and travis about testing .. they both work well together .. in fact you can use scrutinizer to collect and display code coverage generated by travis ci

@rudyrigot
Copy link
Contributor Author

So, the master and this branch are now PSR-2 compliant. It would be nice to keep this going through time indeed; I'll ask the guys, but I think it would be nice to use ScrutinizerCI indeed.

@rudyrigot
Copy link
Contributor Author

We took an entirely different approach (using APC), and the PR for this feature is now #49, and is almost ready to land.

@rudyrigot rudyrigot closed this May 21, 2014
@rudyrigot rudyrigot deleted the cache branch May 21, 2014 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants