php-slack / slack

A simple PHP package for sending messages to Slack, with a focus on ease of use and elegant syntax.
BSD 2-Clause "Simplified" License
135 stars 27 forks source link

Set Content-Type header on requests #70

Closed bytestream closed 3 years ago

bytestream commented 3 years ago

https://github.com/php-slack/slack/blob/006520e96e97292e9bfff2cd8259cf20d37696b8/src/Client.php#L475

The json request option should be used instead of body so that the Content-Type request header is set correctly to application/json

A Content-Type header of application/json will be added if no Content-Type header is already present on the message. https://docs.guzzlephp.org/en/6.5/request-options.html#json

This ensures compatibility with Slack-like services such as rocket chat and mattermost.

cmbuckley commented 3 years ago

One minor impact of using json option would be that Guzzle does not support passing any json_encode options, as per further down on their docs:

This request option does not support customizing the Content-Type header or any of the options from PHP's json_encode() function. If you need to customize these settings, then you must pass the JSON encoded data into the request yourself using the body request option and you must specify the correct Content-Type header using the headers request option.

The Client currently chooses to pass JSON_UNESCAPED_UNICODE into json_encode. Removing this would not be a breaking change, and I agree with the above suggestion, but just wanted to flag this detail.

bytestream commented 3 years ago

I'll change the PR to just set the header directly rather than using the json request option. See https://github.com/php-slack/slack/pull/71/commits/4a32d94098d0964bfef2631d70476c629fc95f67

cmbuckley commented 3 years ago

For now I think this is a little easier yes, as there is also the question of catching and rethrowing the Guzzle exception to maintain a consistent sendMessage interface.

Longer term I have a PR that uses your suggestion above, but would need to be a major version bump unless we caught Guzzle’s exception.