Add http_(get|clear)_last_reponse_headers() functions#12500
Add http_(get|clear)_last_reponse_headers() functions#12500Girgias merged 1 commit intophp:masterfrom
Conversation
Girgias
force-pushed
the
http_last_reponse_header
branch
from
a8d3cde to
6b3936f
Compare
|
@Girgias I think this patch should fix the crashes: diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c
index 60d484341a..bd5efb384c 100644
--- a/ext/standard/basic_functions.c
+++ b/ext/standard/basic_functions.c
@@ -423,6 +423,8 @@ PHP_RINIT_FUNCTION(basic) /* {{{ */
BASIC_RINIT_SUBMODULE(dir)
BASIC_RINIT_SUBMODULE(url_scanner_ex)
+ ZVAL_UNDEF(&BG(last_http_headers));
+
/* Setup default context */
FG(default_context) = NULL;
@@ -489,6 +491,8 @@ PHP_RSHUTDOWN_FUNCTION(basic) /* {{{ */
BASIC_RSHUTDOWN_SUBMODULE(user_filters)
BASIC_RSHUTDOWN_SUBMODULE(browscap)
+ zval_ptr_dtor(&BG(last_http_headers));
+
BG(page_uid) = -1;
BG(page_gid) = -1;
return SUCCESS;
diff --git a/ext/standard/http.c b/ext/standard/http.c
index 6276fdb2d1..921172a305 100644
--- a/ext/standard/http.c
+++ b/ext/standard/http.c
@@ -242,6 +242,6 @@ PHP_FUNCTION(http_last_response_header)
zend_parse_parameters_none();
if (!Z_ISUNDEF(BG(last_http_headers))) {
- ZVAL_DUP(return_value, &BG(last_http_headers));
+ ZVAL_COPY(return_value, &BG(last_http_headers));
}
}
The DUP->COPY change is only a performance improvement. It is not necessary to fix the crashes. |
|
Currently away from home, will try this on Monday :) |
Girgias
force-pushed
the
http_last_reponse_header
branch
from
6b3936f to
c2cebfa
Compare
Girgias
force-pushed
the
http_last_reponse_header
branch
from
c2cebfa to
993059b
Compare
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Code looks correct, and I personally think it's a sensible feature.
But since Jakub is the code owner here I'll let him have the final say.
|
I think this whole thing is an anti-pattern so we should not introduce a special function for that |
|
What I would prefer to see is a function that give headers from the stream resource. Something like |
Girgias
force-pushed
the
http_last_reponse_header
branch
from
993059b to
c7ead31
Compare
Girgias
changed the title
Girgias
force-pushed
the
http_last_reponse_header
branch
from
c7ead31 to
3706db2
Compare
|
@bukka any final remarks before I merge this? |
ext/standard/http.c
Outdated
There was a problem hiding this comment.
You should check the return value, or use the macro version I think. Same comment for further below.
There was a problem hiding this comment.
Indeed, and I didn't initially what you meant .-. so now I'm also using explicit return macros.
ext/standard/http.c
Outdated
There was a problem hiding this comment.
When using RETURN_COPY_VALUE, you should not pass return_value.
There was a problem hiding this comment.
Yeah that was a hastly change that I didn't test locally :/
ext/standard/http.c
Outdated
There was a problem hiding this comment.
Strictly speaking the engine initializes it to NULL for you, so this is not necessary but eh... better be explicit ig.
There was a problem hiding this comment.
Actually this function returns void so RETURN_NULL is confusing here.
Girgias
force-pushed
the
http_last_reponse_header
branch
from
86287ad to
7afadfb
Compare
ext/standard/http.c
Outdated
There was a problem hiding this comment.
Actually this function returns void so RETURN_NULL is confusing here.
This is to provide an alternative to the $http_response_header magic variable
Girgias
force-pushed
the
http_last_reponse_header
branch
from
7afadfb to
741b1d3
Compare
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
I believe it is correct now, let's see what CI says
3 participants
This is to provide an alternative to the magic variable $http_response_header