Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Notifier] add OvhCloud bridge#34540
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
| CHANGELOG | ||
| ========= | ||
| 5.0.0 |
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.
5.1
| /** | ||
| * @author Thomas Ferney <thomas.ferney@gmail.com> | ||
| * | ||
| * @experimental in 5.0 |
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.
5.1
| * @throws \Symfony\Contracts\HttpClient\Exception\ClientExceptionInterface | ||
| * @throws \Symfony\Contracts\HttpClient\Exception\RedirectionExceptionInterface | ||
| * @throws \Symfony\Contracts\HttpClient\Exception\ServerExceptionInterface | ||
| * @throws \Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface |
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 think the@throws annotations can be removed
| /** | ||
| * @author Thomas Ferney <thomas.ferney@gmail.com> | ||
| * | ||
| * @experimental in 5.0 |
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.
5.1
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.
Thank you
| parent::__construct($client, $dispatcher); | ||
| } | ||
| public function setHostByEndpoint(?string $endpoint): self |
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 find the method name confusing. What aboutsetEndpointName() instead?
Uh oh!
There was an error while loading.Please reload this page.
| $now = time() + $this->timeDelta; | ||
| $headers['X-Ovh-Timestamp'] = $now; | ||
| if (isset($this->consumerKey)) { |
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.
if (null === $this->consumerKey) {
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.
If my assumption is right above, the condition must be removed and local variables should be used instead.
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 "consumerKey" is mandatory, so I think I can remove this condition.
If it's empty, it throws an TransportException with "Missing X-Ovh-Consumer header"
If it's undefined in the dsn, it throws an 500 with null instead of a string in __construct()
What do you think?
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.
indeed, that's always set, so you can remove the condition.
| $headers['X-Ovh-Application'] = $this->applicationKey; | ||
| if (!isset($this->timeDelta)) { |
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.
if (null === $this->timeDelta) {
| $response = $this->client->request( | ||
| 'GET', | ||
| $endpoint | ||
| ); |
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.
Should be on one line
| } | ||
| /** | ||
| * Calculate time delta between local machine and API's server. |
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.
Calculates
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.
Calculates the ...
| $headers['X-Ovh-Application'] = $this->applicationKey; | ||
| if (!isset($this->timeDelta)) { | ||
| $this->calculateTimeDelta(); |
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.
IIUC, this should be calculated for every email we sent as this class can be used in a long running process.
| */ | ||
| private function calculateTimeDelta(): int | ||
| { | ||
| if (!isset($this->timeDelta)) { |
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.
Should be removed, if we call this method, it means that we want to compute the value.
| ); | ||
| $serverTimestamp = (int) (string) $response->getContent(); | ||
| $this->timeDelta = $serverTimestamp - (int) time(); |
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 would not set it here, but just return the value and store it in the calling code.
| @@ -0,0 +1,12 @@ | |||
| OvhCloud Notifier | |||
| =============== | |||
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.
Missing=
src/Symfony/Bundle/FrameworkBundle/Resources/config/notifier_transports.xmlShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
3046c5b toc4f05f6Compareb74f610 to76bfb85CompareThank you@antiseptikk. |
This PR was squashed before being merged into the 5.1-dev branch.Discussion----------[Notifier] add OvhCloud bridge| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets | See#33687| License | MITThis would add OvhCloud sms integration for the Notifier component.Tested with 'ovh-eu' entrypoint.Inspiration :https://github.com/ovh/php-ovhCommits-------76bfb85 [Notifier] add OvhCloud bridge
This would add OvhCloud sms integration for the Notifier component.
Tested with 'ovh-eu' entrypoint.
Inspiration :https://github.com/ovh/php-ovh