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

Suggestion to create base class for key-value storages 2 #697

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

afilippov1985
Copy link
Contributor

It is renewed PR #676.

Rewritten storages:

  • Cassandra
  • CouchbaseDB
  • Redis

Added storages:

  • Memcached
  • MongoDB

@danopz
Copy link

danopz commented Jan 12, 2016

I like this approach. Could you add some very descriptive doc-blocks in that change pls. 😄

@bshaffer
Copy link
Owner

This is awesome! a few things

  • we need to convert the tabs to spaces
  • have you considered using traits instead of a base class?

@afilippov1985
Copy link
Contributor Author

Hmm.. I don't see any tabs.
Unfortunately, traits can't implement interfaces. So we can't use instanceof to check whether class implements such interface.

@afilippov1985
Copy link
Contributor Author

though... storage class can declare that it implements interfaces, but actual implementation will be done by trait.
Then ... would be better to create several traits (one for each interface), each will implement corresponding interface.

No, it was a bad idea.

@bshaffer
Copy link
Owner

Yes, that is what I had in mind! This is tricky though because this would
break compatibility with PHP 5.3.
On Wed, Jan 13, 2016 at 3:12 AM afilippov1985 [email protected]
wrote:

though... storage class can declare that it implements interfaces, but
actual implementation will be done by trait.
Then ... would be better to create several traits (one for each
interface), each will implement corresponding interface.


Reply to this email directly or view it on GitHub
#697 (comment)
.

@afilippov1985
Copy link
Contributor Author

Yes, I also wanted to mention about this. Traits require PHP >=5.4

@afilippov1985
Copy link
Contributor Author

Oops... travis not working [https://travis-ci.org/bshaffer/oauth2-server-php/jobs/102334251]

@afilippov1985
Copy link
Contributor Author

Waiting for PR #701

@bluebaroncanada
Copy link
Collaborator

So, with #701, this is passing?

@afilippov1985
Copy link
Contributor Author

Rechecked.
Yes, it passes tests.
Current develop branch also passes tests with #701

@bluebaroncanada
Copy link
Collaborator

So @bshaffer, unless you say otherwise, I'll merge this and #701 in about a week.

), $config);
}

protected function getValue($key)
protected function _makeKey($table, $key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for underscore before method name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed underscores to conform PSR-2 style

@afilippov1985
Copy link
Contributor Author

@bluebaroncanada
I think this code is ready to be merged.
Current installations that uses Cassandra, Couchbase or Redis storage will not notice changes (I hope).

@afilippov1985
Copy link
Contributor Author

@bluebaroncanada @bshaffer
Then I want to discuss #675 (first comment) to make interfaces consistent in v2.x

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.

5 participants