Skip to content

Conversation

voku
Copy link
Contributor

@voku voku commented Jul 21, 2023

ref.: https://github.com/php/php-src/blob/master/ext/curl/multi.c#L60 (stub: function curl_multi_init(): CurlMultiHandle {})

⇒ "Stubs are the authoriative source of truth. The descriptions in the manual are sometimes buggy due to them not being syncronized with the stubs via automatic tooling (unlike signatures)." - Máté Kocsis (https://twitter.com/kocsismate90/status/1681912594024525824)

ref.: https://github.com/php/php-src/blob/master/ext/curl/multi.c#L60 (stub: `function curl_multi_init(): CurlMultiHandle {}`)

⇒ "Stubs are the authoriative source of truth. The descriptions in the manual are sometimes buggy due to them not being syncronized with the stubs via automatic tooling (unlike signatures)." - Máté Kocsis
voku added a commit to voku/phpstorm-stubs that referenced this pull request Jul 21, 2023
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably also adjust curl_init. With regard to consistency curl_share_init does not ist the word "object".

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it is missing en Error/Exception section as I imagine it now throws an error on failure? Or did that just never occur?

@voku
Copy link
Contributor Author

voku commented Jul 21, 2023

@Girgias
Copy link
Member

Girgias commented Jul 21, 2023

Okay so the implementation never fails, but we don't check if it has been properly initialized looking at the cURL docs: https://curl.se/libcurl/c/curl_multi_init.html

@TimWolla
Copy link
Member

curl_init still returns false: https://github.com/php/php-src/blob/master/ext/curl/interface.c#L1200

Fun. I fully expected it to throw and thus didn't check.

@TimWolla TimWolla merged commit 65d5499 into php:master Jul 21, 2023
@TimWolla
Copy link
Member

Merging, as this is a improvement as it is. Thanks!

@voku
Copy link
Contributor Author

voku commented Jul 21, 2023

Okay so the implementation never fails, but we don't check if it has been properly initialized looking at the cURL docs: https://curl.se/libcurl/c/curl_multi_init.html

Return value

If this function returns NULL, something went wrong and you cannot use the other curl functions.


=> Sounds like it doesn't always return a new object. 🤔

nielsdos pushed a commit to nielsdos/doc-en that referenced this pull request Jul 29, 2023
LolGleb pushed a commit to JetBrains/phpstorm-stubs that referenced this pull request Aug 18, 2023
@divinity76
Copy link
Contributor

divinity76 commented Jan 15, 2024

edit: hmm this isn't an issue, this is a PR 🤔
wouldn't exactly call this issue resolved, because now it is /undocumented/ what curl_multi_init() does on failure

@divinity76
Copy link
Contributor

divinity76 commented Jan 15, 2024

related PR php/php-src#13157

fwiw prior to that PR (php <8.4.0-dev?) it returns a broken curl_multi handle on failure

@Girgias
Copy link
Member

Girgias commented Jan 16, 2024

related PR php/php-src#13157

fwiw prior to that PR (php <8.4.0-dev?) it returns a broken curl_multi handle on failure

Could you open a new issue about this, or add an entry to UPGRADING in the php-src PR so this does not get forgotten.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants