-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
Add http_(get|clear)_last_reponse_headers() functions #12500
Conversation
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 :) |
6b3936f
to
c2cebfa
Compare
c2cebfa
to
993059b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
993059b
to
c7ead31
Compare
c7ead31
to
3706db2
Compare
@bukka any final remarks before I merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ext/standard/http.c
Outdated
|
||
PHP_FUNCTION(http_get_last_response_headers) | ||
{ | ||
zend_parse_parameters_none(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, and I didn't initially what you meant .-. so now I'm also using explicit return macros.
ext/standard/http.c
Outdated
} | ||
|
||
if (!Z_ISUNDEF(BG(last_http_headers))) { | ||
RETURN_COPY_VALUE(return_value, &BG(last_http_headers)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using RETURN_COPY_VALUE, you should not pass return_value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that was a hastly change that I didn't test locally :/
ext/standard/http.c
Outdated
|
||
zval_ptr_dtor(&BG(last_http_headers)); | ||
ZVAL_UNDEF(&BG(last_http_headers)); | ||
RETURN_NULL(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this function returns void so RETURN_NULL is confusing here.
86287ad
to
7afadfb
Compare
ext/standard/http.c
Outdated
|
||
zval_ptr_dtor(&BG(last_http_headers)); | ||
ZVAL_UNDEF(&BG(last_http_headers)); | ||
RETURN_NULL(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this function returns void so RETURN_NULL is confusing here.
This is to provide an alternative to the $http_response_header magic variable
7afadfb
to
741b1d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it is correct now, let's see what CI says
This is to provide an alternative to the magic variable $http_response_header