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

Add redis cache support #75

Closed
wants to merge 18 commits into from
Closed

Add redis cache support #75

wants to merge 18 commits into from

Conversation

titanrat
Copy link
Contributor

Add redis cache support using MemCache type as example

@PowerKiKi
Copy link
Member

Thanks for the contribution. You can fix the code style via ./vendor/bin/php-cs-fixer fix.

However there are talk to refactor entirely the caching system in #3 (and also a PR for Redis against PHPExcel). Please join in the discussion and make your opinion known whether PSR-16 support would be an interesting feature. I'll wait until we reach a agreement with @MarkBaker on the cache system future to merge or reject this PR...

@titanrat
Copy link
Contributor Author

Code style fixed. I'll try to join discussion later.

@Quix0r
Copy link
Contributor

Quix0r commented Jan 23, 2017

I would like to see use (which is like impot in Java) instead of having static (full-qualified) class references everywhere:

use \Vendor\Project\Type\Foo;

$foo = new Foo();

Now if \Vendor\Project\Type\Foo has changed to e.g. \Vendor\Project\Type\Fooier\Foo then you have to update all references, with use only in the header in each file.

Edit: I opened an issue for this.

@titanrat
Copy link
Contributor Author

@Quix0r You want me to fix that in my new file CachedObjectStorage\Redis.php ?

@Quix0r
Copy link
Contributor

Quix0r commented Jan 23, 2017

@titanrat I'm no dev here, maybe @PowerKiKi or @MarkBaker can say something about it?

@titanrat
Copy link
Contributor Author

@Quix0r fixed. But only in file which i created.

Copy link
Contributor

@Quix0r Quix0r 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 with me.

@Quix0r
Copy link
Contributor

Quix0r commented Jan 25, 2017

@titanrat fine with me. :-) And maybe the people here won't like your PR then.

@titanrat
Copy link
Contributor Author

@Quix0r Let's see what they say.

@PowerKiKi
Copy link
Member

@Quix0r like I said earlier, I won't merge this until we get a decision in #3 about moving to PSR-16. There are chances that this PR will become useless.

@PowerKiKi
Copy link
Member

This made obsolete by the introduction of PSR-16, thanks for the contribution anyway.

@PowerKiKi PowerKiKi closed this Mar 13, 2017
@titanrat titanrat deleted the RedisCache branch April 28, 2017 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants