Getallheaders fpm 5.6

[ralt]

For @remicollet, mostly :)

To follow up on #904, since the 5.5 branch can't have tests on fpm.

You only need to git cherry-pick 3c16ece after merging #904 into PHP-5.5 and merging it upwards.

[smalyshev]

@ralt looks like this one needs a rebase

[ralt]

@smalyshev just did a rebase against upstream/PHP-5.6... no conflict, then pushed. It seems github still can't merge without conflict.

[remicollet]

I think the test on var_len is broken (when padding is 0)

Please, keep add_request_header (from cgi) unchanged, as this will reduce risk,
Just call, in FPM:

 func((char*)p->arKey, p->nKeyLength-1, *(char**)p->pData, strlen(*(char**)p->pData), arg, TSRMLS_CC);

[hikari-no-yume]

Is there any way to actually preserve the header case? This naïve algorithm won't work properly for certain headers, and would break poorly-written code that relies on header case.

[nosignleft]

I didn't checked for FPM, but doing a "strace" on a FastCGI-only process shows that the headers are transmitted in upper case, no matter what they looked like in the original request. I guess FPM will receive the same data when using mod_fastcgi on Apache.

Might be different with other implementations, like mod_proxy_fcgi (or other web server), but an existing case sensitive code would already not work is most cases.

[hikari-no-yume]

Ah, that's unfortunate :/

OK then. Could this code be shared with the other places in PHP that use the same algorithm? CGI and FastCGI already have way too much copy-pasting.

[cmb69]

What's the status here? Wouldn't it be nice to have this functionality?

Wrt. the merge conflicts: I suggest to leave NEWS out of this (and generally most) PR. The file is changing too rapidly, it's easy to add the relevant line after merging, and last but not least it deserves special treatment when merging upwards, anyway.

[hikari-no-yume]

NEWS is always treated specially, yeah.

[remicollet]

@Tyrael what do you think of merging this in 5.6 ?

(notice, some downstream distro already have such a patch, getallheaders is already there is most SAPI)

[Tyrael]

@remicollet if you think the patch is correct I would be fine having it in 5.6

[cjsfj]

@remicollet Is this patch needed for PHP7? It seems to have some issues on my end with one of the functions having an incorrect set of parameters
main/SAPI.c:1131: error: too many arguments to function 'add_assoc_stringl_ex'

[remicollet]

Yeah... I need to find time to work on this

[cjsfj]

Thanks @remicollet

[cjsfj]

Hey @remicollet - is this needed in php 7.1? This is the only issue that blocked us from upgrading to 7.0. We don't have the expertise to fix the patch :( but are eager to upgrade to 7.1 when released.

[krakjoe]

@ralt @remicollet bump, there are conflicts, and I'm not sure of the status here ?

[hikari-no-yume]

What is this patch? I can't remember any more. If it's a backport of getallheaders() for FPM to 5.6, we can simply close it, because 5.6 is in sec-fix-only mode. If it adds it to FPM in the first place, it'd need to be retargeted to 7.0.

[krakjoe]

There is mention of PHP 7.0 work in the thread, I'm basically waiting on Remi to tell me what's going on.

Am aware this patch as it stands is not useful, but the conversation is ... left open for that reason, will be closed when I get answers.

[hikari-no-yume]

Mm. If it is to be added to 7.x, it could go into a 7.0 patch release.

[krakjoe]

Having waited a month for feedback and for merge conflicts to be resolved - which they must be in order for testing to be possible - and since nobody has responded, I'm closing this PR.

Please take this action as encouragement to open a clean PR against a supported branch.

[AnnoyingTechnology]

I can provide feedback. Please fix this bug, it's been 5 years now https://bugs.php.net/bug.php?id=62596

[remicollet]

For history.... long time.... but #3363