-
Notifications
You must be signed in to change notification settings - Fork 36
Added new API endpoint for "email/unsubscribe" and fixed the response object #22
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
base: master
Are you sure you want to change the base?
Conversation
| * | ||
| * @param array $data | ||
| * @return Response | ||
| */ |
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.
Why not name a method getUnsubscribedUsersCount and drop docblock comment?
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 method removes subscribtions, so a set fits better.
|
|
||
| /** | ||
| * Count the unsubscription statistics of a specific campaign. | ||
| * |
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.
Is there more specific type? In worse case could be @param mixed[] $data.
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.
I don't know what $data contains, so mixed[] should fit for now.
| function __construct(array $result = array()) | ||
| { | ||
| if (!isset($result['replyCode']) || !isset($result['replyText']) || !isset($result['data'])) { | ||
| if (!isset($result['replyCode']) || !isset($result['replyText']) || !array_key_exists('data', $result)) { |
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.
Can you please explain this modification?
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.
I'm not sure why @mwille did this, but I guess this can be dropped (the effect should be the same).
| $this->replyCode = $result['replyCode']; | ||
| $this->replyText = $result['replyText']; | ||
| $this->data = $result['data']; | ||
| $this->data = (array) $result['data']; |
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 will happen here if $result['data'] is not an array?
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 will force $this->data to be an array, it will be empty if $result['data'] is not an array (this might be better than mixed data types).
|
I'm sorry for the late answer. My colleague doesn't work anymore on the project where these changes were required. So I tried to answer your questions. |
|
Thank you for your response. Please update the PR and I will be happy to merge it. |
The new API endpoint depends on the changes in response constructor.