-
Notifications
You must be signed in to change notification settings - Fork 62
Add Support For files.getUploadURLExternal & files.completeUploadExternal #182
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Thanks a lot for this implementation and the new helper function in the client ! I just have one question.
src/Client.php
Outdated
); | ||
} | ||
|
||
private function uploadToSlackUrlWithCurl(string $uploadUrl, string $filePath): void |
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 would be nice to use the http client user in the actual api client (Jane).
Would that be possible?
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 tried it out but couldn't make it work. Ended up moving from cURL and using GuzzleClient instead. Let me know if this is not ok for you.
I got following error with generated jane api client:
1) JoliCode\Slack\Tests\WritingTest::testItCanUploadFilesViaFilesUploadV2
Http\Client\Common\Exception\ClientErrorException: Not Found
/Users/john.doe/repositories/slack-php-api/vendor/php-http/client-common/src/Plugin/ErrorPlugin.php:80
/Users/john.doe/repositories/slack-php-api/vendor/php-http/client-common/src/Plugin/ErrorPlugin.php:62
/Users/john.doe/repositories/slack-php-api/vendor/php-http/httplug/src/Promise/HttpFulfilledPromise.php:28
/Users/john.doe/repositories/slack-php-api/vendor/php-http/client-common/src/Plugin/ErrorPlugin.php:63
/Users/john.doe/repositories/slack-php-api/vendor/php-http/client-common/src/PluginChain.php:44
/Users/john.doe/repositories/slack-php-api/vendor/php-http/client-common/src/PluginChain.php:59
/Users/john.doe/repositories/slack-php-api/vendor/php-http/client-common/src/PluginClient.php:84
/Users/john.doe/repositories/slack-php-api/src/Client.php:193
/Users/john.doe/repositories/slack-php-api/src/Client.php:151
/Users/john.doe/repositories/slack-php-api/tests/WritingTest.php:264
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.
That was discussed here too: #176
I like what you did yes 👍 much more closer to the actual API client that way. I will suggest a modification tho.
src/Client.php
Outdated
$client = Psr18ClientDiscovery::find(); | ||
$response = $client->sendRequest($request); |
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.
We must use the Client provided by the developer, so I think this should be:
$client = Psr18ClientDiscovery::find(); | |
$response = $client->sendRequest($request); | |
$response = $this->httpClient->sendRequest($request); |
The HttpClient is from here:
protected $httpClient; |
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.
That's exactly what I did, it doesn't work at least when I run my test :(
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.
@damienalexandre it doesn't work because generated client has base uri set as slack.com and what getUploadUrlExternal returns is a different url i.e., https://files.slack.com/upload/v1/CwABAAAAYwoAAdZbBYEretOHCgACGDH-HcmojZAMAAMLAAEAAAALRTA3UEI5R1QzTU0LAAIAAAALVTA4S1YyNjkwOEsLAAMAAAALRjA4S1kyQVRFUTYACgAEAAAAAAAAAJoIAAcAAAAEAAsAAgAAABTBhf2hsqHZcysRfVuQ1zXp61dVZAA
and that's why I am getting 404s
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.
Ok I see, I just tested your branch, and found a way to keep the "files.slack.com" URI when using the internal HTTP Client. I just pushed a new commit in your PR to add this. Is it ok for you?
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.
Nice, looks good! 👍
Thanks a lot! This was an highly requested feature! 👏 |
Slack deprecated https://api.slack.com/methods/files.upload and introduced a new flow for file uploads, which comprises of the following steps:
Fixes #168