8000 Changing the parsing of headers reflects in trouble in laravel projects · Issue #47747 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Changing the parsing of headers reflects in trouble in laravel projects #47747

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
Closed
realtebo opened this issue Sep 30, 2022 · 5 comments
Closed

Changing the parsing of headers reflects in trouble in laravel projects #47747

realtebo opened this issue Sep 30, 2022 · 5 comments
Labels
Bug Status: Waiting feedback

Comments

@realtebo
Copy link
realtebo commented Sep 30, 2022 •
edited
Loading

Symfony version(s) affected

6.1.5 (version of symfony/http-foundation)

Description

this was the sintax we used to use.

Note $headers

public function showPdf($id)
{
    $headers = [
        'Content-Type: application/pdf',
    ];

  ... cutted ..

    return response()->file($pathToFile,$headers);
}

This code adds an header named Content-Type with value: application/pdf

This will not work anymore since 6.1.5 of this package

After the upgrade it create an header named 0 with value Content-Type: application/pdf

Since now, only this following syntax works . But we must fix EVERY SINGLE LARAVEL PROJECT

    $headers = [
        'Content-Type' => 'application/pdf',
    ];

How to reproduce

I am able to reproduce only in a laravel project

Possible Solution

I ask for a revert

Additional Context

No response
@realtebo realtebo added the Bug label Sep 30, 2022
@wouterj
Copy link
Member
wouterj commented Sep 30, 2022

Please share what "does not work" means and how you know that this is related to the HttpFoundation component (as you say you can only reproduce in a Laravel project and the code example you showed is not from Symfony).

@realtebo realtebo mentioned this issue Sep 30, 2022
Closed
@stof
Copy link
Member
stof commented Sep 30, 2022

I don't think passing strings like 'Content-Type: application/pdf' ever worked fine. $response->headers->get('Content-Type') already failed before. you were just lucky enough that the wrong usage was not impacting your app in a visible way.
The expected usage of the BinaryFileResponse constructor (or of the Response constructor) has always been to expect an associative array. As #47516 changed some of the Content-Type checks done in BinaryFileResponse, your luck might have ended.
however, maybe #47746 will restore your luck.

@wouterj
Copy link
Member
wouterj commented Oct 2, 2022

I agree with @stof here and closing here as the referenced PR has been merged.

I recommend you still update your projects to use the associative array, as the code you show does indeed not work as intended (in any version of Symfony).

@wouterj wouterj closed this as completed Oct 2, 2022
@realtebo
Copy link
Author
realtebo commented Oct 2, 2022
8000

I don't think passing strings like 'Content-Type: application/pdf' ever worked fine. $response->headers->get('Content-Type') already failed before. you were just lucky enough that the wrong usage was not impacting your app in a visible way. The expected usage of the BinaryFileResponse constructor (or of the Response constructor) has always been to expect an associative array. As #47516 changed some of the Content-Type checks done in BinaryFileResponse, your luck might have ended. however, maybe #47746 will restore your luck.

Abolutely not true.
I was working until the upgrade.

Before:

public function showPdf($id)
{
    $headers = [
        'Content-Type: application/pdf',
    ];

  ... cutted ..

    return response()->file($pathToFile,$headers);
}

will add an header named Content-Type with value: application/pdf

After the upgrade it create an header name 0 with value Content-Type: application/pdf

We were (wrongly, ok,...) using this sintax in a few projects, and was working. Upgrading laravel bring with itself the upgrade of your package and so we digged until we discovered which specific package upgrade broke the things.

I know it's not your fault... but I think it could be usefull for future googler to know.

Also I opened an issue with same goal on laravel/framework, Also, I asked laravel doc team to documented the right way to pass headers because it's not documented at all.
Probably a lot of time ago someone of my team googled and found a working snippet. Is was working for a casuality, for error, but it was working.

In the last 3 days we corrected, and upgraded, all of our project having this misuse.

But absolutely, our syntax worked, and laravel was not altering headers before passing down to httpFoundation package.

@realtebo
Copy link
Author
realtebo commented Oct 2, 2022

I don't think passing strings like 'Content-Type: application/pdf' ever worked fine. $response->headers->get('Content-Type') already failed before. you were just lucky enough that the wrong usage was not impacting your app in a visible way. The expected usage of the BinaryFileResponse constructor (or of the Response constructor) has always been to expect an associative array. As #47516 changed some of the Content-Type checks done in BinaryFileResponse, your luck might have ended. however, maybe #47746 will restore your luck.

As stated, and modified in the first post, it worked. Ok. Was an error. But worked. I was not lossing my and out time if it was not working :)

@wouterj wouterj mentioned this issue Oct 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Waiting feedback
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@stof @wouterj @realtebo @carsonbot
0