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

added MongoDB (new php driver mongodb) storage #748

Closed
wants to merge 4 commits into from

Conversation

AstRonin
Copy link

No description provided.

@AstRonin AstRonin mentioned this pull request Jul 15, 2016
@danopz
Copy link

danopz commented Jul 15, 2016

Ah I've found that #697 adds this storage too

@AstRonin
Copy link
Author

AstRonin commented Jul 15, 2016

Brent has a choice :)

$mongodb = Bootstrap::getInstance()->getMongoDB();
}

public function testSetClientDetails() {
Copy link
Owner

@bshaffer bshaffer Jul 15, 2016

Choose a reason for hiding this comment

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

curly braces after function declarations should be on their own line, to be consistent with the styles in the rest of this library

@bshaffer
Copy link
Owner

This looks promising, but a few major things need to be fixed before we merge this.

  1. Travis tests are failing because we need to add the mongodb.so extension in travis.yml.
  2. You need to fix the line endings so the diffs accurately reflect the changes submitted by this PR. check out dos2unix or this SO article on how to remove the carriage returns.
  3. What is the purpose of MongoDBTest? All these methods are covered by the standard storage suite of tests.

@AstRonin
Copy link
Author

Ok, I will do the changes after my holidays.

  1. I did not have to deal with travis.yml ... what I should do?
  2. Yes, about newlines, I noticed after pushing...
  3. I needed to make sure which methodes work (write/read/remove) as I expected in different situations. For example, method \OAuth2\Storage\Mongo::setRefreshToken does insert instead of update and it looks like a bug. Standart tests don't identify it.

Another thing is that I can change [] to array() in Bootstrap.php for maintain a common style but we cannot use new MongoDB driver with php v5.3

@bshaffer
Copy link
Owner

@AstRonin Sounds good! Okay, to address your questions:

  1. To deal with travis.yml, add the following lines to before_install:

    before_install:
    - if [[ ${TRAVIS_PHP_VERSION:0:3} == "7.0" ]]; then
        phpenv config-add testing/mongodb.ini;
      elif [[ ${TRAVIS_PHP_VERSION:0:3} != "5.3" ]]; then
        pecl install mongodb || /bin/true;
      fi
  2. great!

  3. Ok I get it. If there's a way to test all storage classes for this, that would be best. If it's a mongodb-specific thing to test, then this works great. We should remove duplicate tests though.

  4. This is no problem! Have Bootstrap::getMongoDB use NullStorage in this case, and the tests will be skipped:

    if ('5.3' === substr(phpversion(), 0, 3)) {
        $this->mongodb = new NullStorage('MongoDB', 'The mongodb.so extension is not compatible with PHP 5.3');
    }

@AstRonin
Copy link
Author

AstRonin commented Sep 7, 2016

I added new commit, but travis failed again...


public function testAuthorizationCode()
{
$db = new MongoDB('mongodb://localhost:27017/oauth2_server_php');
Copy link

@scaleupcto scaleupcto Sep 26, 2016

Choose a reason for hiding this comment

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

Test fails because Call to undefined method MongoDB\Driver\Manager::setAuthorizationCode() in /home/travis/build/bshaffer/oauth2-server-php/test/OAuth2/OpenID/Storage/AuthorizationCodeTest.php on line 77.

I think you need to be more specific here and use the namespace new \OAuth2\Storage\ MongoDB because the error suggests you are trying to call the setAuthorizationCode on the underlying MongoDB driver class, which obv won't work.

@AstRonin
Copy link
Author

AstRonin commented Sep 27, 2016

ok, it's my fault, I mixed classes in Bootstrap in previous test...
Now error only for :

  • PHP 5.3 - MongoDB does not work in 5.3 at all, but Trevis parsed anyway...
  • HHVM - Has no extension MongoDB

Will I need add something else in travis.yml?

@svycka
Copy link
Contributor

svycka commented Sep 27, 2016

php5.3 really? http://php.net/supported-versions.php

@AstRonin
Copy link
Author

AstRonin commented Sep 27, 2016

@scaleupcto
Copy link

HHVM works but you need the specific mongodb.so for that env.

@chadicus
Copy link
Contributor

If anyone is interested, I created companion library over the weekend. https://github.com/chadicus/oauth2-storage-mongodb.
I also needed to use MongoDB for storage, but the current restrictions of this library, it was easier to create a separate storage implementation rather than try to get a PR approved here. As soon as this library ups its version of PHP, I'll be creating a PR.

Please keep in mind my mongodb library is in its very early stages and I'm sure it has bugs. I would not recommend for production use at this time.

@bshaffer
Copy link
Owner

bshaffer commented Jan 6, 2017

Added in #790

@bshaffer bshaffer closed this Jan 6, 2017
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.

6 participants