-
Notifications
You must be signed in to change notification settings - Fork 57
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
Swapped to Vonage namespace #50
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found some bugs in the php code snippets (some of them might be caused with underlying issues in the SDK and should be looked into)
|
||
$secret = 'awes0meNewSekret!!;'; | ||
|
||
$client->account()->createSecret(NEXMO_API_KEY, $secret); | ||
$client->account()->createSecret(VONAGE_API_KEY, $secret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This snippet does not work, and it appears that there's a bug in the SDK causing it to throw a not very helpful error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is due to that instance bug with the accounts API. This should be fixed in Vonage/vonage-php-sdk-core#248 but hasn't been released yet as we found it right after 2.3.1 was cut.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed this works with v2.3.2 which was just released, but please double-check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in good shape @dragonmantank and my compliments also to @slorello89 for a thorough review. I added a couple of questions (then I realised I was asking the same question lots of times and stopped) but with that either answered or changed, this is pretty much ready to go.
The other issues @slorello89 spotted are valid but shouldn't be in this PR. Let's grab them and tun them into (excellent starter) issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a mission :) Looks good to me!
Everything is swapped from Nexmo -> Vonage