slack-php icon indicating copy to clipboard operation
slack-php copied to clipboard

7/update jul2020

Open bobeagan opened this issue 4 years ago • 7 comments

This fixes #5, fixes #7

Change Summary

  • remove php end tag; php recommended best practice
  • add install section for composer
    • also include require line for autoload script with example
  • update endpoints
    • there were a few endpoints that no longer exist, according to the docs
    • added many new ones that were missing from the list
    • included deprecation note for various endpoints, per slack api docs
    • update method to match preferred method from slack api docs

bobeagan avatar Jul 21 '20 23:07 bobeagan

@palanik went through all the Slack API docs to sync them up here. If there are any changes you need me to make just let me know. Thanks!

bobeagan avatar Jul 21 '20 23:07 bobeagan

@bobeagan Thanks for the PR. I will review & get back to you.

palanik avatar Jul 22 '20 00:07 palanik

@bobeagan I see that you switched to POST method for some endpoints to match preferred method from slack api docs. This will break if the endpoint is called with no body. The underlying library expects a mandatory body as the last argument for all POST, PUT or PATCH methods.

For e.g. api->test()

palanik avatar Jul 22 '20 19:07 palanik

Running a local test of this, seems like there are some issues with this library using POST and working properly (could also be that I'm doing something wrong). I'll continue to investigate, probably best not to merge this until that is worked out.

bobeagan avatar Jul 22 '20 19:07 bobeagan

Didn't see your message above before drafting my latest comment. Thanks for the direction.

bobeagan avatar Jul 22 '20 19:07 bobeagan

Alright, I ran a quick test of chat.scheduledMessages.list and here is what happened:

  • Request (no body): $client->chat->scheduledMessages->list();

    • Response: { ["ok"]=> bool(true) ["scheduled_messages"]=> array(0) { } ["response_metadata"]=> array(1) { ["next_cursor"]=> string(0) "" } }
  • Request (array): $client->chat->scheduledMessages->list(['channel' => 'C123456789']);

    • Response: { ["ok"]=> bool(true) ["scheduled_messages"]=> array(0) { } ["warning"]=> string(15) "missing_charset" ["response_metadata"]=> array(2) { ["next_cursor"]=> string(0) "" ["warnings"]=> array(1) { [0]=> string(15) "missing_charset" } } }
  • Request (json_encoded array): $client->chat->scheduledMessages->list('{"channel":"C123456789"}');

    • Response: { ["ok"]=> bool(false) ["error"]=> string(17) "missing_post_type" }

It appears that the Content-Type needs to be explicitly passed when using POST with JSON-encoded bodies. Additionally. the token should be included as a bearer token in the Authorization header. (https://api.slack.com/web#json-encoded_bodies)

We cannot, however, simply treat all POST as json encoded due to files.upload which only supports multipart/form-data or application/x-www-form-urlencoded Content-Types.

Maybe allowing to replace the $opts would let me create multiple clients that served different purposes or having helper functions that constructed them appropriately? Let me know your thoughts or if I'm missing something.

bobeagan avatar Jul 22 '20 20:07 bobeagan

The right fix to me seems to be to allow endpoints themselves to define their Guzzle options/headers but that would require an update to wrapi-php. The shortcut workaround seems like allowing the constructor to accept an optional options array after the token which when provided would replace the default one. Then people would just need to create different client instances based on what endpoints they were using it for. This still would result in a breaking change since the formatting of the payload sent in the request changes (array vs json encoded string) due to the update to the method on several of the endpoints. Let me know if you are interested in moving forward with some change to fix this or not. Thanks.

bobeagan avatar Jul 23 '20 17:07 bobeagan