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

Invalid conversion between multiple Laravel headers and Swoole headers #341

Closed
filakhtov opened this issue Feb 18, 2021 · 3 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@filakhtov
Copy link
Contributor

  1. Your software version (Screenshot of your startup)

    Software Version
    PHP 7.4.14
    Swoole 4.5.9
    Laravel/Lumen 8.26.1
  2. Detail description about this issue(error/log)

Laravel supports multiple response headers with the same name (as per HTTP standard). It is achieved by calling HeaderBag::set() with an array as a second argument, something like:

$request->headers->set('trailer', ['a', 'b']);

This is accounted for in LaravelS, i.e. it iterates over all header values and calls Swoole\Http\Response::header() method. However, this usage is incorrect, according to the Swoole documentation, namely:

  • 重复设置相同 $key 的 HTTP 头会覆盖,取最后一次

which Google Translate helped me with:

  • repeating the same set of $key HTTP head Will overwrite, take the last

i.e., in normal words, only the last value will be preserved.

  1. Some reproducible code blocks and steps
$response = new Response();
$response->headers->set('X-My-Header', ['a', 'b', 'c']);
return $response;

When this snippet put into any controller the result will be:

> GET / HTTP/1.1
> Host: localhost
> User-Agent: curl/7.64.1
> Accept: */*

< HTTP/1.1 200 OK
< Server: LaravelS
< Date: Thu, 18 Feb 2021 05:36:17 GMT
< Content-Type: text/html
< Content-Length: 0
< X-My-Header: c

but the expected outcome should have multiple X-My-Header headers.

Perhaps, we should look at header folding? Or is that something for Swoole to enable support for?

@filakhtov filakhtov added the analyzing Analyzing this issue label Feb 18, 2021
@filakhtov filakhtov changed the title Invalid conversion between Laravel headers and Swoole headers Invalid conversion between multiple Laravel headers and Swoole headers Feb 18, 2021
@hhxsv5 hhxsv5 added bug Something isn't working and removed analyzing Analyzing this issue labels Feb 19, 2021
@hhxsv5
Copy link
Owner

hhxsv5 commented Feb 19, 2021

Swoole (<4.6.0) does not support setting the header key with the same name,
LaravelS supports it now, but you need to upgrade Swoole to 4.6.x.
A new version of LaravelS will be released in the next few days.

@hhxsv5 hhxsv5 closed this as completed in f8710f8 Feb 19, 2021
@hhxsv5
Copy link
Owner

hhxsv5 commented Feb 24, 2021

v3.7.17

@filakhtov
Copy link
Contributor Author

Thank you very much! We have finally got around to properly testing it, as we were blocked by swoole/swoole-src#4133 when we first tried everything together, but once Swoole actually addressed their issue everything is working properly now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants