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

ErrorHandler - Present more error info about MySQL connections #224

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

totten
Copy link
Member

@totten totten commented Oct 9, 2024

Overview

Present more error info about MySQL connections. This may be most useful if you're debugging a failure
in MySQL-SSL.

Technical Details

This is a work-around for a quirk in DB_mysqli::connect() -- it suppresses warnings
generated by mysqli_real_connect(). But those can be useful!

Th work-around depends on your boot mode (Civi-first vs CMS-first).
It applies to Civi-first. For CMS-first, the CMS error-handler is active
at the critical moment -- and it doesn't have such a work-around.

I'm on the fence about merging this. This should be safe enough (very narrow
impact). But it would be cleaner (and probably better) to patch
DB_mysqli::connect() to simply allow warnings. (Or, failing that, buffer
the warnings -- and let them through if we hit any subsequent problems.)
Addressing that root cause would make it less sensitive to boot-mode.

Of course, warnings have been suppressed for so long... it's hard to know if
we might create some backlash from it...

…around)

Overview
--------

Present more error info about MySQL connections. This may be most useful if you're debugging a failure
in MySQL-SSL.

Technical Details
-----------------

This is a work-around for a quirk in `DB_mysqli::connect()` -- it suppresses warnings
generated by `mysqli_real_connect()`. But those can be useful!

Th work-around depends on your boot mode (Civi-first vs CMS-first).
It applies to Civi-first. For CMS-first, the CMS error-handler is active
at the critical moment -- and it doesn't have such a work-around.

I'm on the fence about merging this. This is safe enough (very narrow
impact). But it would be cleaner (and probably better) to patch
`DB_mysqli::connect()` to simply allow warnings. (Or, failing that, buffer
the warnings -- and let them through if we hit any subsequent problems.)
Addressing that root cause would make it less sensitive to boot-mode.

Of course, warnings have been suppressed for so long... it's hard to know if
we might create some backlash from it...
@demeritcowboy
Copy link
Contributor

I'd lean towards removing the @. I'm always leaning towards that.

But also is this a php 7 vs 8 thing? See red box at https://www.php.net/manual/en/language.operators.errorcontrol.php

@totten
Copy link
Member Author

totten commented Oct 9, 2024

I'd lean towards removing the @. I'm always leaning towards that.

It is evil...

But also is this a php 7 vs 8 thing? See red box at https://www.php.net/manual/en/language.operators.errorcontrol.php

Oh, good link. I think that means the problem is generally worse on php7. But for MySQL SSL errors (specifically), it reports the messages with E_WARNING. So the @ operator suppresses them on both php7+php8.

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.

2 participants