-
Notifications
You must be signed in to change notification settings - Fork 14
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
RHS1b3 New push.admin.device_registrations.save #65
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.
Looking good but please see comments.
src/Models/DeviceDetails.php
Outdated
|
||
class DeviceDetails extends BaseOptions { | ||
|
||
const DevicePushTransportType = ['fcm', 'gcm', 'apns', 'web']; |
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.
We've tended not to include this kind of validation check in the library, because then if we do add support for new types it's not possible to access that functionality without having a new library. I think it's best that the backend performs this validation.
/** | ||
* @var string | ||
*/ | ||
public $metadata; |
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.
In the IDL this is declared to be of type JsonObject
/** | ||
* @var string | ||
*/ | ||
public $push; |
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.
In the IDL this is declared to be of type DevicePushDetails
src/Models/DeviceDetails.php
Outdated
|
||
if ($this->platform && ! in_array($this->platform, self::DevicePlatform)) { | ||
throw new \InvalidArgumentException( | ||
sprintf('unexpected form factor %s', $this->platform) |
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.
s/form factor/platform, but I guess this code is being removed anyway
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.
Done, could you 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.
LGTM, thanks
RHS1b3 New push.admin.device_registrations.save