Getallheaders fpm

[ralt]

Following up on #523 , fixed the comments by refactoring and fixing the trailing character.

Reminder: fixes https://bugs.php.net/bug.php?id=62596

(Since it's a follow-up, cc @krakjoe @TazeTSchnitzel @manuelm)

[remicollet]

As this PR introduce a new function, it will be nice to include a unit test (5.6+)

[ralt]

@remicollet indeed, will do. I learned less than an hour ago that fpm had tests now! :) Thanks for your work on this.

[ralt]

As discussed together, I will make another PR for master, since some functions are not compatible anymore, namely add_assoc_stringl_ex.

[remicollet]

And FYI, I just add 017.phpt which is the first one to run some specific code inside the FPM server.

[ralt]

Hmmm how should I handle the fact that this PR is for php-5.5, but the tests are for php-5.6 only?

[remicollet]

apache_request_headers / getallheaders functions are available in nsapi, apache*, cli server, cgi and litespeed, so make sense for FPM.

All other SAPI use PHP_FALIAS(getallheaders, apache_request_headers,
So probably better to do the same (switch alias / function name)

[remicollet]

I also think having cgi_add_request_header and fpm_add_request_header is a bit tricky
Why not simply only use sapi_add_request_header (of course with the correct val_len value) ?

Hmmm how should I handle the fact that this PR is for php-5.5, but the tests are for php-5.6 only?

We'll just have to be careful during the merge (probably merging all in 5.6 and cherry-pick only the revelant commit in 5.5)

[ralt]

All other SAPI use PHP_FALIAS(getallheaders, apache_request_headers,
So probably better to do the same (switch alias / function name)

Will do.

Why not simply only use sapi_add_request_header (of course with the correct val_len value) ?

How do I pass the correct value? It's used as a function callback, so I need an intermediary function.

We'll just have to be careful during the merge (probably merging all in 5.6 and cherry-pick only the revelant commit in 5.5)

I'll let you handle it then :-)

[ralt]

The failed tests don't really seem related to this PR...

[ralt]

@remicollet I fixed:

• The alias orders
• The duplication (it uses sapi_add_request_header right away)

The PR #910 adds the missing test.

[cjsfj]

Seems like this change normalizes the case of the headers, as well as leaves off the last character. At least that's what it did when I patched my source.

Shouldn't the headers come through as-is rather than the case changed?

[hikari-no-yume]

It'd be better to preserve case, definitely. For the CLI server getallheaders() I made it case-preserving, as trying to "correct" HTTP header names both mangles input and requires a list of HTTP headers which must be kept current.

[hikari-no-yume]

In its case, all headers were stored in an array with lowercase keys for retrieval. The solution was to add a second array with original-case keys which pointed to the same values.

[cjsfj]

Of course keeping a standard list of http headers is impossible in the case where developers are adding custom headers in their web applications.

@TazeTSchnitzel So in this case, normalization is probably the best policy when dealing with fpm. Do you agree? If so I think Title case is the wrong approach -- something more like you've implemented with cli (lower case) would be more appropriate.

[hikari-no-yume]

I'm not familiar with the intricacies of FastCGI, but are you sure case normalisation is the only way to go? Bear in mind this won't work properly anyway as the rules have exceptions.

[cjsfj]

Honestly my understanding of this type of development is very elementary. I just wanted to post and indicate that while this pull request is submitted, when I applied it to my source, the resulting getallheaders output had the following issues:

• The keys were all missing the last character
• The case of the keys do not match when compared to running the same function on an instance using the Apache module.

Thanks for your attention. If I'm in err please let me know.

[cmb69]

It seems this PR can be closed, because PHP 5.5 is in security support mode, and there's the follow-up PR #910 which targets PHP 5.6.