Fixed php7 build using wrapper for curl_getinfo #868
Conversation
|
All is green! :) |
| @@ -197,6 +197,6 @@ public static function convertRequestToCurlCommand(Request $request) | |||
| */ | |||
| public static function debugEnabled() | |||
| { | |||
| return defined('DEBUG') && DEBUG; | |||
| return defined('ELASTICA_DEBUG') && ELASTICA_DEBUG; | |||
There was a problem hiding this comment.
I didn't rename the constant on purpose as this will break BC. If we change it, it must be mentioned as BC break in the CHANGELOG. I suggest we change it as I think in the long term it makes more sense to have it in the namespace.
There was a problem hiding this comment.
Hmm, haven't thought if this is BC. In that case, I would suggest to remove this ELASTICA_DEBUG completely, because debug should be an parameter of connection constructor and not some magic constant.
There was a problem hiding this comment.
Agree. I'm not even sure if "debug" is the correct name. Perhaps it is more "verbose" or something like that.
There was a problem hiding this comment.
Though, only difference I see between "debug mode" and not "debug mode" is just two things:
- Would
$response->setQueryTimebe invoked after request. - Would curl option
CURLINFO_HEADER_OUTbe applied or not.
Both of them has no impact on performance, is this really worth to keep debug option?
There was a problem hiding this comment.
I remember it was more a memory issue then a performance issue. CURLINFO_HEADER_OUT returns the full string as part of the response object. If you use bulk mode this can add up to quite a large object. CURLINFO_HEADER_OUT could also be set through the curl options.
The query time part should not have an affect and I think this is what is mostly needed.
Best would be to active the benchmark tests to see if there is a difference. They should work now again but are not enabled for travis.
There was a problem hiding this comment.
They should work
I tried, but they don't.
Then I would like to remove this constant. One who want to get headers in response could pass CURLINFO_HEADER_OUT via curl options.
|
👍 Can you update the CHANGELOG? |
- Response::getQueryTime is always available now. - Use 'curl' parameter of Transport\Http to specify option CURLINFO_HEADER_OUT=true, if you want to have "request_header" key in Response::getTransferInfo
im-denisenko
force-pushed
the
php7-segfault
branch
from
57425d5 to
0366e15
Compare
|
@ruflin Changelog updated, debug removed, bc break mentioned. |
Fixed php7 build using wrapper for curl_getinfo
|
Thx. Nice one as complexity reduced. We could mention in the CHANGELOG more specific that the curl params now must be used. |
Was discussed here: #861 (comment)