Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Notifier] Add missing charset to content-type for Slack notifier#41211

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Nyholm merged 1 commit intosymfony:5.2fromnorkunas:add-missing-charset-for-slack
May 13, 2021

Conversation

@norkunas
Copy link
Contributor

QA
Branch?5.2
Bug fix?yes
New feature?no
Deprecations?no
TicketsN/A
LicenseMIT
Doc PRN/A

Symfony HttpClient doesn't set charset for the Content-Type header when used withjson request option so with each response slack includes:

"response_metadata" => array:1 [  "warnings" => array:1 [    0 => "missing_charset"  ]]

@OskarStark
Copy link
Contributor

@nicolas-grekas is this expected behavior of http client?

Copy link
Contributor

@OskarStarkOskarStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could you please add a test case to avoid a regression in the future?

and maybe check other transports which are using the 'json' key?

@nicolas-grekas
Copy link
Member

Yes it is:application/json is an utf8-only content-type. The API is stupid...

OskarStark reacted with thumbs up emojiNyholm reacted with laugh emoji

@norkunas
Copy link
ContributorAuthor

Could you please add a test case to avoid a regression in the future?

and maybe check other transports which are using the 'json' key?

Will add a test.

I think this is specific only for slack, as by spec json must be in utf-8 so adding charset everywhere shouldn't be required.

nicolas-grekas and OskarStark reacted with thumbs up emoji

@OskarStark
Copy link
Contributor

Fine thank you 👍🏻

Copy link
Contributor

@OskarStarkOskarStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

After a test was added

@norkunasnorkunasforce-pushed theadd-missing-charset-for-slack branch from1de0473 toe642100CompareMay 13, 2021 08:16
@norkunas
Copy link
ContributorAuthor

After a test was added

Done ;)

OskarStark reacted with thumbs up emoji

@Nyholm
Copy link
Member

Good catch, thanks Tomas.

norkunas reacted with rocket emoji

@NyholmNyholm merged commit35dbf8c intosymfony:5.2May 13, 2021
@norkunasnorkunas deleted the add-missing-charset-for-slack branchMay 13, 2021 09:59
This was referencedMay 19, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@OskarStarkOskarStarkOskarStark approved these changes

@NyholmNyholmNyholm approved these changes

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

Assignees

No one assigned

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

5 participants

@norkunas@OskarStark@nicolas-grekas@Nyholm@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp