Amazon, Fitbit, Heroku, Paypal, Stripe Clients added #63
Conversation
|
Thanks for this @luchianenco! Just give us a few days to review it - it's on our list :). |
|
|
||
| public function getProviderClass(array $config) | ||
| { | ||
| return 'Luchianenco\OAuth2\Client\Provider\Amazon'; |
There was a problem hiding this comment.
I'm just wondering why not using the syntax Amazon::class?
There was a problem hiding this comment.
well, I tried to keep the style the repository had before ;)
of course it can be a next PR ;) to change in all Configurators
There was a problem hiding this comment.
Oh, yeah, maybe @weaverryan can tell us the true reason behind this :)
There was a problem hiding this comment.
I think it's just legacy from the past, but since we require PHP >=5.5.9 in composer.json now changing to ::class seems reasonable to me, but yes, in another PR and for all occurrences
There was a problem hiding this comment.
Uh oh, probably I was wrong! The main problem is that we do not have strict dependencies on those packages, i.e. user installs them if needed. So, it'll throw an error if the class does not exist. That's why here we use strings which postpone this check until actual call. Sorry for misleading you in my previous message 😇
There was a problem hiding this comment.
Ahh, that makes sense, thanks for the clarification
|
Only one minor comment, but everything looks great Thanks @luchianenco! |
|
|
||
| public function getProviderClass(array $config) | ||
| { | ||
| return '\AdamPaterson\OAuth2\Client\Provider\Stripe'; |
There was a problem hiding this comment.
We don't need leading slash, i.e. AdamPaterson\OAuth2\Client\Provider\Stripe
| public function buildConfiguration(NodeBuilder $node) | ||
| { | ||
| $node | ||
| ->scalarNode('is_sandbox') |
There was a problem hiding this comment.
Looks like it should be booleanNode() instead, right?
README.md
Outdated
| redirect_route: connect_paypal_check | ||
| redirect_params: {} | ||
| # When true, client uses Paypal Sandbox urls. | ||
| # is_sandbox: '' |
There was a problem hiding this comment.
This should be false as value by default instead of an empty string
There was a problem hiding this comment.
bin/update_readme uses '' instead of boolean value. I will updated it manually but after next usage to update readme it will be again is_sandbox: ''
There was a problem hiding this comment.
Hm, I thought it should be false after scalarNode() -> booleanNode() tweak
There was a problem hiding this comment.
I have updated update_readme to set boolean values...please check if it is fine
bin/update_readme
Outdated
|
|
||
| } elseif ($child instanceof BooleanNode) { | ||
| $defaultValue = $child->getDefaultValue() | ||
| ? $child->getDefaultValue() |
There was a problem hiding this comment.
Should be simply 'true', otherwise we'll have 1 in README instead
There was a problem hiding this comment.
do u mean : $defaultValue = $child->getDefaultValue() ? 'true'
There was a problem hiding this comment.
Not quite, I mean ? 'true' on this line, i.e.:
$defaultValue = $child->getDefaultValue()
? 'true'
: 'false';We need true as a string too
There was a problem hiding this comment.
that's what I meant ;) sorry just shorted the expression :)
ok I will do
|
Thanks for the |
|
Woohoo! Sorry for the delay - but this is awesome! I'll tag a new release shortly :) |
|
@weaverryan I can understand :) too many events |
|
Haha, yea... a bit of traveling... and things are pretty busy right now with Symfony 4 prep. Busy... but exciting :) Cheers! |
No description provided.