[Notifier] add OvhCloud bridge #34540
Conversation
| CHANGELOG | ||
| ========= | ||
|
|
||
| 5.0.0 |
| /** | ||
| * @author Thomas Ferney <thomas.ferney@gmail.com> | ||
| * | ||
| * @experimental in 5.0 |
| * @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.
I think the @throws annotations can be removed
| /** | ||
| * @author Thomas Ferney <thomas.ferney@gmail.com> | ||
| * | ||
| * @experimental in 5.0 |
| parent::__construct($client, $dispatcher); | ||
| } | ||
|
|
||
| public function setHostByEndpoint(?string $endpoint): self |
There was a problem hiding this comment.
I find the method name confusing. What about setEndpointName() instead?
| $now = time() + $this->timeDelta; | ||
| $headers['X-Ovh-Timestamp'] = $now; | ||
|
|
||
| if (isset($this->consumerKey)) { |
There was a problem hiding this comment.
if (null === $this->consumerKey) {
There was a problem hiding this comment.
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.
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.
indeed, that's always set, so you can remove the condition.
|
|
||
| $headers['X-Ovh-Application'] = $this->applicationKey; | ||
|
|
||
| if (!isset($this->timeDelta)) { |
| $response = $this->client->request( | ||
| 'GET', | ||
| $endpoint | ||
| ); |
| } | ||
|
|
||
| /** | ||
| * Calculate time delta between local machine and API's server. |
| $headers['X-Ovh-Application'] = $this->applicationKey; | ||
|
|
||
| if (!isset($this->timeDelta)) { | ||
| $this->calculateTimeDelta(); |
There was a problem hiding this comment.
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.
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.
I would not set it here, but just return the value and store it in the calling code.
| @@ -0,0 +1,12 @@ | |||
| OvhCloud Notifier | |||
| =============== | |||
src/Symfony/Bundle/FrameworkBundle/Resources/config/notifier_transports.xml
Show resolved
Hide resolved
3046c5b to
c4f05f6
Compare
b74f610 to
76bfb85
Compare
|
Thank 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 | MIT This would add OvhCloud sms integration for the Notifier component. Tested with 'ovh-eu' entrypoint. Inspiration : https://github.com/ovh/php-ovh Commits ------- 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