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

fix got null some time #552

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

Conversation

WitzHsiao
Copy link

I found a bug when I get Access Token from DynamoDB, which will got null very some time.

@WitzHsiao WitzHsiao changed the title fix got null sometime fix got null some time Mar 27, 2015
@bshaffer
Copy link
Owner

bshaffer commented Apr 1, 2015

I feel like the ConsistentRead option could be supplied to the DynamoDB storage on creation. This would allow the user of the application to decide if they wanted to use consistent reads or not.

Eventual Consistency most likely would be acceptable in most cases.

@WitzHsiao
Copy link
Author

sounds like a plan!

@bshaffer
Copy link
Owner

bshaffer commented Apr 9, 2015

is this something you are willing to add in another Pull Request?

@bshaffer
Copy link
Owner

@WitzHsiao are you planning on submitting a PR for this change?

@WitzHsiao
Copy link
Author

@bshaffer Sorry for reply lately. I've changed it to get ConsistenRead from config of DynamoDB.

@bshaffer
Copy link
Owner

These should all read

"ConsistentRead" => $this->config['consistent_read'] 

With the consistent_read parameter being defined here and defaulting to false

@bshaffer
Copy link
Owner

Also, there's a typo. All of these say ConsistenRead instead of ConsistentRead.

Also, we need tests.

@bshaffer bshaffer added this to the v1.8 milestone Apr 27, 2015
@bshaffer
Copy link
Owner

@WitzHsiao pinging on this open issue

@WitzHsiao
Copy link
Author

@bshaffer Sorry, forgot this issue. Thanks for review. I have already fix the typo in the second commit.

@bshaffer
Copy link
Owner

bshaffer commented May 1, 2017

This is not what I had in mind. Rather than using isset each time, you can have the parameter defaulted in the constructor. i.e here:

        $this->config = array_merge(array(
            'client_table' => 'oauth_clients',
            'access_token_table' => 'oauth_access_tokens',
            'refresh_token_table' => 'oauth_refresh_tokens',
            'code_table' => 'oauth_authorization_codes',
            'user_table' => 'oauth_users',
            'jwt_table'  => 'oauth_jwt',
            'scope_table'  => 'oauth_scopes',
            'public_key_table'  => 'oauth_public_keys',
            'consistent_read' => false, // <---- HERE
        ), $config);

@bluebaroncanada bluebaroncanada modified the milestones: v1.10.0, v1.8 Aug 15, 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.

None yet

3 participants