-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix #16929: curl_getinfo($ch, CURLINFO_CONTENT_TYPE) returns false when Content-Type header is not set #16997
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
Conversation
...to false - Ensures consistent behavior when Content-Type is missing
ondrejcech
commented
Nov 30, 2024
@MarcusXavierr I believe that returning NULL
values is better because false
is returned in case of error as described in php docs.
If option is given, returns its value. Otherwise, returns an associative array with the following elements (which correspond to option), or false on failure:
Returning false
would mean the call ended with an error, but curl_error()
is empty.
@ondrejcech yep, it seems it should return NULL when someone calls curl_getinfo($ch, CURLINFO_CONTENT_TYPE)
image
But should this function call return NULL just to this option? Because all string responses from curl_getinfo seem to have false
as a fallback
Lines 2663 to 2670 in ba8e3e1
Edit: I think adding a new case on the switch is the cleanest solution 🤔
ondrejcech
commented
Nov 30, 2024
I think returning false
is confusing because it makes it difficult to distinguish between a fallback value and an actual error.
I agree, that adding a new case on the switch is the cleanest for my problem.
Looks like this is already pretty inconsistent. In some cases false
is returned, in other cases the array element is not there. Now, returning null
only in this case might not be the best idea. I'm not sure, though, what's the best way forward.
ondrejcech
commented
Dec 2, 2024
OK, I understand. Can we at least make curl_getinfo($ch, CURLINFO_CONTENT_TYPE)
consistent with the result of curl_getinfo($ch)['content_type']
? The first will return false
, the second NULL
.
@cmb69 that's the point. from what I've read on the code, if the request was successful, but the Content-Type
was missing, the only possible response from now on is NULL
. And wouldn't the content_type
array key not be set only when there's an error on curl_easy_getinfo(ch->cp, CURLINFO_CONTENT_TYPE, &s_code)
?
Lines 2496 to 2504 in ba8e3e1
I had a closer look: as is, curl_getinfo()
with $option
either returns the value or false
. curl_getinfo()
without $option
only set array elements which have a value. Now changing curl_getinfo($ch, CURLINFO_CONTENT_TYPE)
to return null
if there is no Content-Type header would be inconsistent with the other options. Changing curl_getinfo($ch)
to set the content_type
to false would likely do more harm than good (code using isset()
works fine for missing elements as well as null
elements).
Thinking about that further, the different behavior regarding returning false
and not setting the array element is not really inconsistent; think of curl_getinfo()
without $option
as curl_getinfo_all()
; a separate function could easily justify the behavior.
The only real inconsistency I see is that the content_type
element is always set. That is the result of fixing https://bugs.php.net/46739, which, in my opinion, should have been fixed in the docs. Now the docs are still wrong (not all these elements are necessarily set in the array). "Unfixing" https://bugs.php.net/46739 after 16 years might not be the best idea, though.
Uh oh!
There was an error while loading. Please reload this page.
This PR addresses an inconsistency in how
curl_getinfo($ch, CURLINFO_CONTENT_TYPE)
handles cases where the Content-Type header is missing or empty. Reported on issue #16929Previously, curl_getinfo could return
false
when using the specific CURLINFO_CONTENT_TYPE flag, butNULL
when accessing the content_type key from the associative array returned by curl_getinfo($ch).Now, it'll return
NULL
in both situations.