Add support for Slack's "Blocks" and OAuth instead of webhooks #53
Conversation
…king backwards compatibility
soullivaneuh
left a comment
soullivaneuh
left a comment
There was a problem hiding this comment.
Hi and thanks for your contribution!
Can you please give a documentation link explaining the deprecated stuff and how to migrate?
README.md
Outdated
|
|
||
| ```php | ||
| $message = $this->slackClient->createMessage() | ||
| ->setText('This is the fallback text') |
There was a problem hiding this comment.
Too much indentation here.
| return $this; | ||
| } | ||
|
|
||
| public function setOptions(array $options, string $endpoint = self::SLACK_POST_MESSAGE_URL, HttpClient $httpClient = null): self |
There was a problem hiding this comment.
Could you please explain why you created a setOptions method? I don't see the interest as the options can be defined on the constructor.
There was a problem hiding this comment.
It's a bit of a balancing act trying to not break backward-compatibility.
Slack has deprecated webhook endpoints so ideally endpoint would just be an option with a sensible default. But unfortunately, the constructor has it as the first parameter.
The two ways I see this being called are the old web-hook way:
$client = new Nexy\Slack\Client('https://hooks.slack.com/...', $options);
And the new oauth way:
$client = (new Nexy\Slack\Client())->setOptions($options);
I guess I could get rid of the setOptions function and then people would have to instantiate like this:
$client = new Nexy\Slack\Client(Nexy\Slack\Client::SLACK_POST_MESSAGE_URL, $options);
But I kinda thought that was a little yuck.
| { | ||
| $this->options = $this->optionsResolver->resolve($options); | ||
| $this->setEndpoint($endpoint, $httpClient); | ||
| return $this; |
There was a problem hiding this comment.
I don't think we need this.
| */ | ||
| final class Client | ||
| { | ||
| const SLACK_POST_MESSAGE_URL = 'https://slack.com/api/chat.postMessage'; |
There was a problem hiding this comment.
So the API structure is the same as web-hook but with a common URL now, am I right?
| 'unfurl_media' => true, | ||
| 'allow_markdown' => true, | ||
| 'markdown_in_attachments' => [], | ||
| 'oauth_token' => null |
There was a problem hiding this comment.
token should be enough.
There was a problem hiding this comment.
I prefer oauth_token as it matches what it's called in the Slack documentation and interface, and there are many other types of tokens available in Slack:
- OAuth Access Token
- Bot User OAuth Access Token
- Verification Token
And also
- App ID
- Client ID
- Client Secret
- Signing Secret
| ->setAllowedTypes('unfurl_media', 'bool') | ||
| ->setAllowedTypes('allow_markdown', 'bool') | ||
| ->setAllowedTypes('markdown_in_attachments', 'array') | ||
| ->setAllowedTypes('oauth_token', ['string', 'null']) |
There was a problem hiding this comment.
Should be mandatory. Maybe it can be set on the constructor instead to be an option.
There was a problem hiding this comment.
It's not mandatory so as to not break backward compatibility.
README.md
Outdated
|
|
||
| ### Instantiate the client | ||
|
|
||
| Using Slack's [OAuth token](https://api.slack.com/messaging/sending) method: |
There was a problem hiding this comment.
The link does not seem to show oauth usage directly.
| */ | ||
| private $blockId; | ||
|
|
||
| public function __construct(?string $type) |
There was a problem hiding this comment.
Not necessary, the strict typing is enough.
Except if you want to add detail about $type description.
|
Also, can we please have some tests about the new introduced feature? |
|
Re. backward compatibility. I was thinking these changes could be released as a minor version change, then then I could tidy up the constructor signature to something more sensible as a breaking change in a new major version change. |
I'll try and find some motivation for writing tests, but..... it's not my strength ;) |
|
Hi @mikeymclellan, The next release will be a major one, so don't worry about BC break. But you may update the UPGRADE note I just added. Thanks |
|
@mikeymclellan bump ! :-) Do you have some time to finish this merge request? Otherwise, could you please give me edit access? |
|
Sorry for the VERY slow reply @soullivaneuh 😊 I've granted you access now. |
3 participants
No description provided.