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

Update to current Mongo PHP Library #769

Closed
wants to merge 3 commits into from
Closed

Update to current Mongo PHP Library #769

wants to merge 3 commits into from

Conversation

memmaker
Copy link

We also added the missing implementations for the JTI part.

@chadicus
Copy link
Contributor

chadicus commented Oct 18, 2016

Since the minimum PHP requirement for this library is >=5.3.9 and the minimum requirement for the mongodb library is 5.4, @bshaffer may not be too quick to merge this PR. That being said, I am working on a component of sorts for implementing MongoDB storage. You can find the library here. When the minimum PHP requirement of this library in increased, I will put a PR here to add the new storage.

My library is in it's very early stages and should not be used for production code at this time.

@memmaker
Copy link
Author

This is a bit irritating. Did the Travis build fail for my PR or did it not?

@chadicus
Copy link
Contributor

@memmaker your build is failing because the homepage for your author's entry is incorrect. You have http://lrx-solutions,de Note the comma before the de

@@ -1,5 +1,5 @@
{
"name": "bshaffer/oauth2-server-php",
"name": "memmaker/oauth2-server-php",
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert the changes made to composer.json

Copy link
Owner

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! This is great.

There are a few changes requested, and then we can merge it.

), $config);
}

// Helper function to access a MongoDB collection by `type`:
protected function collection($name)
{
return $this->db->{$this->config[$name]};
//return $this->db->{$this->config[$name]};
Copy link
Owner

Choose a reason for hiding this comment

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

remove comment here

OpenIDAuthorizationCodeInterface
{
protected $db;
protected $config;

public function __construct($connection, $config = array())
{
if ($connection instanceof \MongoDB) {
if ($connection instanceof MongoDB\Database) {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's go ahead and add this to a use statement above.

@bshaffer
Copy link
Owner

These changes use the mongodb.so extension instead of the mongo.so extension, so this can be separated in to OAuth2/Storage/MongoDB for BC and clarity!

@chadicus
Copy link
Contributor

chadicus commented Dec 23, 2016

@bshaffer you could avoid BC changes by componentizing some of the library such as the mongodb storage. Users could pick and choose what storage implementation they wanted. Also it would be easier to test the smaller libraries.

@chadicus
Copy link
Contributor

@memmaker I created this library as a temporary solution until the new MongoDB code was added to the main oauth2 server code. Feel free to use any of it's code. I'll be abandoning it once the new MongoDB storage is merged here.

@bshaffer
Copy link
Owner

I added this for BC and also fixed the tests: #790 PTAL!

I agree separate libraries would be better! I will definitely consider splitting these out.

@bshaffer bshaffer closed this Dec 28, 2016
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.

3 participants