Skip to content

[BrowserKit] Raw body with custom Content-Type header #36839

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 22, 2020
Merged

[BrowserKit] Raw body with custom Content-Type header #36839

merged 1 commit into from
May 22, 2020

Conversation

azhurb
Copy link
Contributor

@azhurb azhurb commented May 16, 2020 •
edited
Loading

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
License MIT

Currently, if you try to send POST/PUT request with custom Content-Type header and specified body, the real request will contain text/plain content type.

Following code

$client->request(
    'POST',
    '/url',
    [],
    [],
    [
        'CONTENT_TYPE' => 'application/json'
    ],
    '{"foo":"bar"}'
);

produces next request

POST /
Content-Type: text/plain; charset=utf-8

{"foo":"bar"}

With this fix, the request will be

POST /
Content-Type: application/json

{"foo":"bar"}

@ro0NL
Copy link
Contributor

ro0NL commented May 16, 2020

Hi @azhurb, looks like it fixes #36640 :) cool.

WDYT of my patch suggestion in the issue? I believe there are some other cases where we need to preserve the given content-type as well.

nicolas-grekas
nicolas-grekas reviewed May 16, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks. Here is some nitpicking.

@azhurb
Copy link
Contributor Author

azhurb commented May 16, 2020

Hi @azhurb, looks like it fixes #36640 :) cool.

WDYT of my patch suggestion in the issue? I believe there are some other cases where we need to preserve the given content-type as well.

Hi @ro0NL. Actually I believe we need to preserve content-type only if the raw body data is not empty. It looks like other cases are already covered. Anyway, your patch should work as well :)

@azhurb azhurb requested a review from nicolas-grekas May 16, 2020 17:33
@azhurb
Copy link
Contributor Author

azhurb commented May 16, 2020

There is also discussion from 2016 about this issue #20042

@nicolas-grekas nicolas-grekas added this to the 4.4 milestone May 18, 2020
@fabpot fabpot force-pushed the raw_body_in_request branch from c789964 to d2dd92b Compare May 22, 2020 17:28
@fabpot
Copy link
Member

fabpot commented May 22, 2020

Thank you @azhurb.

@fabpot fabpot merged commit 6d7c696 into symfony:4.4 May 22, 2020
This was referenced May 26, 2020
Merged
Merged
Merged
kbond added a commit to zenstruck/browser that referenced this pull request Dec 2, 2020
@kbond
adjust symfony/browser-kit min version to fix bug
555e41a 
ref symfony/symfony#36839
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
BrowserKit Bug Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
5 participants
@azhurb @ro0NL @fabpot @nicolas-grekas @carsonbot