Skip to content
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

Search for curl_init() only in root namespace #1632

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

mfb
Copy link
Contributor

@mfb mfb commented Nov 7, 2023

If curl is missing at runtime, the curl_init() function call searches the Sentry\HttpClient namespace, which results in a confusing error message: "Call to undefined function Sentry\HttpClient\curl_init()". We only want to search the root namespace.

@mfb
Copy link
Contributor Author

mfb commented Nov 7, 2023

This triggered a code style rule that functions shouldn't have the leading backslash, but it's quite useful at least here IMHO.

@stayallive
Copy link
Collaborator

stayallive commented Nov 7, 2023

Hmm. In theory we require ext-curl so this should never happen.

In practice; I do understand the error is unfortunate here. Might be better to throw a RuntimeException('The cURL extension is missing.') (if !function_exists('curl_init')) or something along those lines which might even be more descriptive yet (and make the CS rule happy)?

Edit: When this is not installed via composer because it's bundled in a plugin like for WordPress this can also easily happen where this requirement cannot be enforced by composer so it would be good to throw an exception so it's logged but won't break.

@mfb
Copy link
Contributor Author

mfb commented Nov 7, 2023

Hmm. In theory we require ext-curl so this should never happen.

Those platform requirements are for composer only. Meanwhile, the app, let's say it's a WordPress site, could have a long lifespan after composer runs, with PHP extensions being enabled and disabled in a control panel, code being moved around, etc.

I updated the branch to allow the leading backslash on curl_init(), so at least the existing error message is improved.

@cleptric
Copy link
Member

cleptric commented Nov 7, 2023

We catch all exceptions thrown in the client here https://github.com/getsentry/sentry-php/blob/master/src/Transport/HttpTransport.php#L86-L95

I'm also not in favour of checking the requirement during runtime, this can cause a bad day for people deploying stuff when we suddenly throw exceptions because cURL is not installed.

Before we add hacks to the code style, let's add a new logger entry, please.

@cleptric
Copy link
Member

cleptric commented Nov 7, 2023

if (!function_exists('curl_init')) {
    throw new \RuntimeException('The cURL extension must be enabled to use the HttpClient.');
}

Put this below the DSN check and we're good.

Or maybe even !extension_loaded('curl').

Copy link
Member

@cleptric cleptric left a comment

Choose a reason for hiding this comment

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

Thanks!

@cleptric cleptric merged commit 1988283 into getsentry:master Nov 7, 2023
32 of 33 checks passed
@mfb
Copy link
Contributor Author

mfb commented Nov 7, 2023

I was assuming that extension_loaded is faster than function_exists() - it has less to search, but idk :)

@cleptric
Copy link
Member

cleptric commented Nov 7, 2023

Yeah, makes sense, good change!

@stayallive
Copy link
Collaborator

A few years ago it made negligible difference in speed so extension_loaded seems more descriptive here 👍

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants