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

CaBundle::getSystemCaRootBundlePath why? #504

Closed
blaaat opened this issue Nov 14, 2018 · 12 comments
Closed

CaBundle::getSystemCaRootBundlePath why? #504

blaaat opened this issue Nov 14, 2018 · 12 comments
Labels
bug Confirmed bug released This feature/bug fix has been released

Comments

@blaaat
Copy link

blaaat commented Nov 14, 2018

When testing with a custom set_error_handler.
I noticed dozens of supressed warnings from Composer Ca Bundle.

Wondering why it's necessary to search the cabundle path on each request instead of setting guzzle verify to true and let guzzle search the capath when necessary.

@tomlongridge tomlongridge added bug Confirmed bug needs discussion Requires internal analysis/discussion labels Jul 24, 2019
@abigailbramble abigailbramble added the scheduled Work is starting on this feature/bug label Aug 27, 2019
@abigailbramble abigailbramble added backlog We hope to fix this feature/bug in the future and removed needs discussion Requires internal analysis/discussion scheduled Work is starting on this feature/bug labels Sep 24, 2019
@GrahamCampbell
Copy link
Contributor

I don't think this is a bug. It's normal for PHP code to have suppressed errors, in the same way that it's normal for code to throw and then catch exceptions.

@blaaat
Copy link
Author

blaaat commented Dec 22, 2019

The issue is not that warnings are suppressed. The issue is that this code is run in the first place. The folder searches are not good for performance.

This code should only run when necessary (when an error is sent to bugsnag)

@GrahamCampbell
Copy link
Contributor

The warnings are suppressed, just suppressed warnings get passed to error handlers by PHP anyway. You need to adjust your handler to check if the current error is suppressed.

@blaaat
Copy link
Author

blaaat commented Dec 22, 2019

I understand that. But this code shouldn’t run every request. Completely unnecessary waste of performance.

@GrahamCampbell
Copy link
Contributor

GrahamCampbell commented Dec 22, 2019

Guzzle would only have to search the disk in the same way if we didn't do it with composer, in general, or worse, would just fail to verify on some systems. Bugsnag needs to support as many different systems as possible.

NB In my profile, searching the disk for certificates actually took last time than executing the rest of the Guzzle constructor.

@blaaat
Copy link
Author

blaaat commented Dec 22, 2019

It might be fast; in my case all paths were protected by an shared-host open base dir configuration. When I reported this issue, this took a significant performance hit (don't have the numbers anymore).

I would suggest constructing Guzzle only when necessary as well (and possible others). The Bugsnag code should have as little impact as possible.

@GrahamCampbell
Copy link
Contributor

I would suggest constructing Guzzle only when necessary as well (and possible others). The Bugsnag code should have as little impact as possible.

I actually looked into doing exactly this today, to see if we could avoid the overhead, but the design of the session stuff which was added more recently makes it hard to illuminate properly. See the issue I made 8 hours ago: #558. :)

Basically, I tried to replace the Guzzle param to the bugsnag constructor with either a Guzzle instance, or a callback that created it, which works, but then there's no easy trick to do the same for the session guzzle instance, that isn't confusing for users (at least, not one that came to me straight away, anyway).

@GrahamCampbell
Copy link
Contributor

I suppose I could still implement this optimisation, which would help if people aren't using the session stuff. I'll (literally) sleep on it.

@bugsnagbot bugsnagbot added scheduled Work is starting on this feature/bug and removed backlog We hope to fix this feature/bug in the future labels Feb 14, 2020
@mattdyoung
Copy link

Fix for PHP >= 5.6 released in v3.20.0

@mattdyoung mattdyoung added released This feature/bug fix has been released and removed scheduled Work is starting on this feature/bug labels Feb 28, 2020
@GrahamCampbell
Copy link
Contributor

It inherently is not possible to fix this entirely on PHP 5.5 without turning off certificate validation.

@GrahamCampbell
Copy link
Contributor

PHP 5.5 users can pass a custom guzzle client to the bugsnag client constructor with a CA file pre-provided if they want to avoid these silenced errors. However, in general silenced errors are common place in PHP code, just like catch blocks are, so it is usual for people to ignore them.

@GrahamCampbell
Copy link
Contributor

It is not a waste of performance now, since in the latest release, we only search when it is strictly necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug released This feature/bug fix has been released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants