Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Notifier] Bring consistency to bridges#50308
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedMay 12, 2023
| Q | A |
|---|---|
| Branch? | 6.3 |
| Bug fix? | no |
| New feature? | no |
| Deprecations? | no |
| Tickets | - |
| License | MIT |
| Doc PR | - |
| /** | ||
| * @return $this | ||
| */ | ||
| publicfunctiondiffusionName(int$diffusionName):static |
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.
it's a bit weird to me that an option named "diffusionName" only accepts integers
can you please confirm this is correct@gnito-org?
nicolas-grekas commentedMay 12, 2023
The TL;DR of this PR is that options shouldn't have getters when they're not required, and their setters should not start with The rest is more CS-related. /cc@OskarStark@alamirault since you contribute to this component. 🙏 |
| thrownewInvalidArgumentException(sprintf('The "From" number "%s" is not a valid phone number. The number must be in E.164 format.',$this->from)); | ||
| $options['from'] =$message->getFrom() ?:$this->from; | ||
| $options['to'] = [$message->getPhone()]; | ||
| $options['accountId'] ??=$this->accountId; |
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.
Before we used account_id kn the request, now accountId, is it correct or did I overlooked sth?
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.
The option array should be aligned with the API. The API usesaccountId, that's why.
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedMay 13, 2023
I'm merging to fix the CI and because this is ready to me, but feel free to comment after the merge if needed! |