Skip to content

[Notifier] [GoogleChat] [BC BREAK] Rename threadKey parameter to thread_key + set parameter via ctor #39579

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 15, 2021
Merged

[Notifier] [GoogleChat] [BC BREAK] Rename threadKey parameter to thread_key + set parameter via ctor #39579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Symfony/Component/Notifier/Bridge/GoogleChat/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ CHANGELOG
---

* The bridge is not marked as `@experimental` anymore
* [BC BREAK] Remove `GoogleChatTransport::setThreadKey()` method, this parameter should now be provided via the constructor,
which has changed from:
`__construct(string $space, string $accessKey, string $accessToken, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)`
to:
`__construct(string $space, string $accessKey, string $accessToken, string $threadKey = null, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)`
* [BC BREAK] Rename the parameter `threadKey` to `thread_key` in DSN

5.2.0
-----
Expand Down
33 changes: 10 additions & 23 deletions src/Symfony/Component/Notifier/Bridge/GoogleChat/GoogleChatTransport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,46 +32,33 @@ final class GoogleChatTransport extends AbstractTransport
private $space;
private $accessKey;
private $accessToken;

/**
* @var ?string
*/
private $threadKey;

/**
* @param string $space The space name the the webhook url "/v1/spaces/<space>/messages"
* @param string $accessKey The "key" parameter of the webhook url
* @param string $accessToken The "token" parameter of the webhook url
* @param string $space The space name the the webhook url "/v1/spaces/<space>/messages"
* @param string $accessKey The "key" parameter of the webhook url
* @param string $accessToken The "token" parameter of the webhook url
* @param string|null $threadKey Opaque thread identifier string that can be specified to group messages into a single thread.
* If this is the first message with a given thread identifier, a new thread is created.
* Subsequent messages with the same thread identifier will be posted into the same thread.
* {@see https://developers.google.com/hangouts/chat/reference/rest/v1/spaces.messages/create#query-parameters}
*/
public function __construct(string $space, string $accessKey, string $accessToken, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)
public function __construct(string $space, string $accessKey, string $accessToken, string $threadKey = null, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)
{
$this->space = $space;
$this->accessKey = $accessKey;
$this->accessToken = $accessToken;

parent::__construct($client, $dispatcher);
}

/**
* Opaque thread identifier string that can be specified to group messages into a single thread.
* If this is the first message with a given thread identifier, a new thread is created.
* Subsequent messages with the same thread identifier will be posted into the same thread.
*
* @see https://developers.google.com/hangouts/chat/reference/rest/v1/spaces.messages/create#query-parameters
*/
public function setThreadKey(?string $threadKey): self
{
$this->threadKey = $threadKey;

return $this;
parent::__construct($client, $dispatcher);
}

public function __toString(): string
{
return sprintf('googlechat://%s/%s%s',
$this->getEndpoint(),
$this->space,
$this->threadKey ? '?threadKey='.urlencode($this->threadKey) : ''
$this->threadKey ? '?thread_key='.urlencode($this->threadKey) : ''
);
}

Expand Down
16 changes: 13 additions & 3 deletions src/Symfony/Component/Notifier/Bridge/GoogleChat/GoogleChatTransportFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\Notifier\Bridge\GoogleChat;

use Symfony\Component\Notifier\Exception\IncompleteDsnException;
use Symfony\Component\Notifier\Exception\UnsupportedSchemeException;
use Symfony\Component\Notifier\Transport\AbstractTransportFactory;
use Symfony\Component\Notifier\Transport\Dsn;
Expand All @@ -22,7 +23,7 @@
final class GoogleChatTransportFactory extends AbstractTransportFactory
{
/**
* @param Dsn $dsn Format: googlechat://<key>:<token>@default/<space>?threadKey=<thread>
* @param Dsn $dsn Format: googlechat://<key>:<token>@default/<space>?thread_key=<thread>
*
* @return GoogleChatTransport
*/
Expand All @@ -37,11 +38,20 @@ public function create(Dsn $dsn): TransportInterface
$space = explode('/', $dsn->getPath())[1];
$accessKey = $this->getUser($dsn);
$accessToken = $this->getPassword($dsn);
$threadKey = $dsn->getOption('threadKey');

$threadKey = $dsn->getOption('thread_key');

/*
* Remove this check for 5.4
*/
if (null === $threadKey && null !== $dsn->getOption('threadKey')) {
throw new IncompleteDsnException('GoogleChat DSN has changed since 5.3, use "thread_key" instead of "threadKey" parameter.');
}

$host = 'default' === $dsn->getHost() ? null : $dsn->getHost();
$port = $dsn->getPort();

return (new GoogleChatTransport($space, $accessKey, $accessToken, $this->client, $this->dispatcher))->setThreadKey($threadKey)->setHost($host)->setPort($port);
return (new GoogleChatTransport($space, $accessKey, $accessToken, $threadKey, $this->client, $this->dispatcher))->setHost($host)->setPort($port);
}

protected function getSupportedSchemes(): array
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Notifier/Bridge/GoogleChat/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ DSN example
-----------

```
GOOGLE_CHAT_DSN=googlechat://ACCESS_KEY:ACCESS_TOKEN@default/SPACE?threadKey=THREAD_KEY
GOOGLE_CHAT_DSN=googlechat://ACCESS_KEY:ACCESS_TOKEN@default/SPACE?thread_key=THREAD_KEY
```

where:
Expand Down
7 changes: 4 additions & 3 deletions src/Symfony/Component/Notifier/Bridge/GoogleChat/Tests/GoogleChatTransportFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ public function createProvider(): iterable
];

yield [
'googlechat://chat.googleapis.com/AAAAA_YYYYY?threadKey=abcdefg',
'googlechat://abcde-fghij:kl_mnopqrstwxyz%3D@chat.googleapis.com/AAAAA_YYYYY?threadKey=abcdefg',
'googlechat://chat.googleapis.com/AAAAA_YYYYY?thread_key=abcdefg',
'googlechat://abcde-fghij:kl_mnopqrstwxyz%3D@chat.googleapis.com/AAAAA_YYYYY?thread_key=abcdefg',
];
}

Expand All @@ -46,7 +46,8 @@ public function supportsProvider(): iterable

public function incompleteDsnProvider(): iterable
{
yield 'missing credentials' => ['googlechat://chat.googleapis.com/AAAAA_YYYYY'];
yield 'missing credentials' => ['googlechat://chat.googleapis.com/v1/spaces/AAAAA_YYYYY/messages'];
Copy link
Member

@jderusse jderusse Jan 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is wrong, as our DSN should not contains v1/spaces the exception might not be related to the missing credentials

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should then be fixed in 5.2 branch, but I think you are right 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a PR: #39848

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR merged and this one rebase @jderusse

yield 'using old option: threadKey' => ['googlechat://abcde-fghij:kl_mnopqrstwxyz%3D@chat.googleapis.com/AAAAA_YYYYY?threadKey=abcdefg', 'GoogleChat DSN has changed since 5.3, use "thread_key" instead of "threadKey" parameter.']; // can be removed in Symfony 5.4
}

public function unsupportedSchemeProvider(): iterable
Expand Down
8 changes: 4 additions & 4 deletions src/Symfony/Component/Notifier/Bridge/GoogleChat/Tests/GoogleChatTransportTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,15 @@ final class GoogleChatTransportTest extends TransportTestCase
/**
* @return GoogleChatTransport
*/
public function createTransport(?HttpClientInterface $client = null): TransportInterface
public function createTransport(?HttpClientInterface $client = null, string $threadKey = null): TransportInterface
{
return new GoogleChatTransport('My-Space', 'theAccessKey', 'theAccessToken=', $client ?: $this->createMock(HttpClientInterface::class));
return new GoogleChatTransport('My-Space', 'theAccessKey', 'theAccessToken=', $threadKey, $client ?: $this->createMock(HttpClientInterface::class));
}

public function toStringProvider(): iterable
{
yield ['googlechat://chat.googleapis.com/My-Space', $this->createTransport()];
yield ['googlechat://chat.googleapis.com/My-Space?thread_key=abcdefg', $this->createTransport(null, 'abcdefg')];
}

public function supportedMessagesProvider(): iterable
Expand Down Expand Up @@ -125,8 +126,7 @@ public function testSendWithOptions()
return $response;
});

$transport = $this->createTransport($client);
$transport->setThreadKey('My-Thread');
$transport = $this->createTransport($client, 'My-Thread');

$sentMessage = $transport->send(new ChatMessage('testMessage'));

Expand Down