Skip to content
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

7/update jul2020 #8

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Conversation

bobeagan
Copy link
Contributor

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 added 3 commits July 21, 2020 16:41
also include require line for autoload script with example
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
Copy link
Contributor Author

@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!

@palanik
Copy link
Owner

palanik commented Jul 22, 2020

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

@palanik palanik changed the base branch from master to dev July 22, 2020 00:05
@palanik
Copy link
Owner

palanik commented Jul 22, 2020

@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()

@bobeagan
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@bobeagan
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slack API Jul2020 Updates Not available through composer
2 participants