[PATCH] Add apache_request_headers() and apache_response_headers() to the CLI server

11 years ago by Andrea Faulds — view source

unread

Hello all,

I implemented FR #65917 (getallheaders() is not supported by the
built-in web server). FR #65917 is actually my own feature request; I
created it because I had tried to use it with the CLI server to test an
application of mine, and it wasn't available. So I went and implemented
it, and since apache_response_headers() complements it and was trivial
to implement as well, I have also implemented that, by copying the
FastCGI code.

Pull request here: https://github.com/php/php-src/pull/496

The intent is for this to be merged into master (and hopefully 5.6). The
pull request updates NEWS and UPGRADING, as well as adding a test.

Thanks for your time.

--
Andrea Faulds
http://ajf.me/

11 years ago by Pierre Joye — view source

unread

hi!

Hello all,

I implemented FR #65917 (getallheaders() is not supported by the built-in
web server). FR #65917 is actually my own feature request; I created it
because I had tried to use it with the CLI server to test an application of
mine, and it wasn't available. So I went and implemented it, and since
apache_response_headers() complements it and was trivial to implement as
well, I have also implemented that, by copying the FastCGI code.

Pull request here: https://github.com/php/php-src/pull/496

The intent is for this to be merged into master (and hopefully 5.6). The
pull request updates NEWS and UPGRADING, as well as adding a test.

Thanks for your time.

Good idea, it was missing for too long already.

I do not think we need a RFC for this addition as it is about bringing
the builtin webserver in sync with other SAPIs.

Cheers,

Pierre

@pierrejoye | http://www.libgd.org

11 years ago by Leigh — view source

unread

Hello all,

I implemented FR #65917 (getallheaders() is not supported by the built-in
web server). FR #65917 is actually my own feature request; I created it
because I had tried to use it with the CLI server to test an application of
mine, and it wasn't available. So I went and implemented it, and since
apache_response_headers() complements it and was trivial to implement as
well, I have also implemented that, by copying the FastCGI code.

Sorry for a late response. I wanted to add my personal feeling is that
I do not like apache_* functions (or function aliases) existing
outside of the Apache SAPI.

getallheaders() yes
apache_* no

I decided to click the pull request link before hitting send. Looks
like this has already been brought up. Going to hit send anyway :)

11 years ago by Andrea Faulds — view source

unread

Sorry for a late response. I wanted to add my personal feeling is that
I do not like apache_* functions (or function aliases) existing
outside of the Apache SAPI.

That's fair enough, but as I had mentioned in the pull request's
discussion, that battle has already been fought and won, as the FastCGI
SAPI also implements it.

And yes, it was already brought up. Not sure why I'm responding either.

--
Andrea Faulds
http://ajf.me/

11 years ago by Yasuo Ohgaki — view source

unread

Hi,

Sorry for a late response. I wanted to add my personal feeling is that
I do not like apache_* functions (or function aliases) existing
outside of the Apache SAPI.

That's fair enough, but as I had mentioned in the pull request's
discussion, that battle has already been fought and won, as the FastCGI
SAPI also implements it.

And yes, it was already brought up. Not sure why I'm responding either.

I'm not following the discussion, but if name matters why don't you use
generic name
function and alias of it?

Generic/same name for the same feature is much better.
Is there problem having alias?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Andrea Faulds — view source

unread

I'm not following the discussion, but if name matters why don't you use
generic name
function and alias of it?

There's already a generic alias of apache_request_headers(), namely
getallheaders(). There's not yet one for apache_response_headers(), but
I don't think it's within the scope of this request to add one. I might
add one with a separate request, mind.

--
Andrea Faulds
http://ajf.me/

11 years ago by Yasuo Ohgaki — view source

unread

I'm not following the discussion, but if name matters why don't you use
generic name
function and alias of it?

There's already a generic alias of apache_request_headers(), namely
getallheaders(). There's not yet one for apache_response_headers(), but I
don't think it's within the scope of this request to add one. I might add
one with a separate request, mind.

getallheaders() is not good name since it does not follow current naming
convention.
Why don't we have

sapi_request_headers()
sapi_response_headers()

http_request_headers()
http_response_headers()

Just an idea.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Andrea Faulds — view source

unread

getallheaders() is not good name since it does not follow current naming
convention.

But it already exists. Why add another alias? We should make
apache_response_headers() have a consistent alias, something like
getresponseheaders(). Then we'll have:

apache_request_headers() and getallheaders()
apache_response_headers() and getresponseheaders()

Under your proposal, we'd have:

apache_request_headers(), sapi_request_headers(), and getallheaders()
apache_response_headers() and sapi_response_headers()

I'd much rather go with my own proposal. It avoids introducing a new set
of aliases, but instead completes the existing one, however imperfect it
may seem.

Cheers,

Andrea Faulds
http://ajf.me/

11 years ago by Yasuo Ohgaki — view source

unread

Hi Andrea,

But it already exists. Why add another alias? We should make
apache_response_headers() have a consistent alias, something like
getresponseheaders(). Then we'll have:

apache_request_headers() and getallheaders()

apache_response_headers() and getresponseheaders()

Under your proposal, we'd have:

apache_request_headers(), sapi_request_headers(), and getallheaders()

apache_response_headers() and sapi_response_headers()

It is not good idea introducing non-standard function name in new releases.
getallheaders() should be depreciated in the future as it does not conform
current naming convention.

How about this?

PHP 5.6: Document getallhaeders() as deprecated.
PHP 5.7: Raise deprecated error for getallheaders().
PHP 6.x: Remove getallheaders() adoption is done. (Perhaps 5 or 10 years
from now?)

It would be nicer cleaning up mess in the long run, rather than
introducing new mess. IMHO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Ferenc Kovacs — view source

unread

Hi Andrea,

But it already exists. Why add another alias? We should make
apache_response_headers() have a consistent alias, something like
getresponseheaders(). Then we'll have:

apache_request_headers() and getallheaders()

apache_response_headers() and getresponseheaders()

Under your proposal, we'd have:

apache_request_headers(), sapi_request_headers(), and getallheaders()

apache_response_headers() and sapi_response_headers()

It is not good idea introducing non-standard function name in new releases.
getallheaders() should be depreciated in the future as it does not conform
current naming convention.

How about this?

PHP 5.6: Document getallhaeders() as deprecated.
PHP 5.7: Raise deprecated error for getallheaders().
PHP 6.x: Remove getallheaders() adoption is done. (Perhaps 5 or 10 years
from now?)

It would be nicer cleaning up mess in the long run, rather than
introducing new mess. IMHO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

I would stick with the original proposal as it is a well understood and
self-contained feature with neglible/no BC break.
Handling the (possible) deprecation and removal of the apache_ functions
should be a separate topic(if you start walking down on that path, you have
to also consider other functions like apache_child_terminate which is also
provided by the cgi-fastcgi SAPI) and should be given a bit more thought
and because of the BC implications it is also a longer process(as you also
mentioned yourself).
Thanks for your feedback!

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

11 years ago by Rowan Collins — view source

unread

Yasuo Ohgaki wrote (on 23/10/2013):

It is not good idea introducing non-standard function name in new releases.
getallheaders() should be depreciated in the future as it does not conform
current naming convention.

While I would tend to agree that adding more "badly" named
functions/aliases would be unfortunate, I would have thought removing
the "bad" name would risk opening a rather large can of worms.

From the prominently linked blog post on RFCs
[https://blogs.oracle.com/opal/entry/the_mysterious_php_rfc_process]:

Don't start an RFC (or mail list discussion) about standardizing PHP
function names and function argument orders. [...] This has been
discussed ad infinitum. Review previous discussions and feel free to
fork PHP on github.

IMHO, there would need to be a pretty strong argument for this
particular function to be permanently renamed without reopening that
discussion for all the other poorly named functions in core. (Note: this
is NOT an invitation to reopen that discussion!)

Perhaps a better idea (which already came up recently) would be to
create a new module for dealing with SAPI I/O (request body data,
environment vars, cookies, etc) and include HTTP header handling in
that. This could also allow better symmetry for setting response
headers, to replace header(), in the same way as setcookie()/$_COOKIE.

Regards,

Rowan Collins
[IMSoP]

11 years ago by Levi Morrison — view source

unread

I also would vote for fixing this in the long-term. Regardless, I
appreciate everyone's work to make the built-in webserver better. Thank you.