Force utf8mb4 query to throw exception as the check expects

[mfb]

Fixes dev/core#749

Overview

The utf8mb4 compatibility check should throw and catch an exception, but instead is throwing a fatal error.

Before

See bug report at https://lab.civicrm.org/dev/core/issues/749

After

Hopefully it will now throw an exception? But needs testing.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mattwire]

Yes this fixes the issue. One thing I don't understand is why we have to assign the result to a variable ie. $errorScope. Without that assign it seems like the call to CRM_Core_TemporaryErrorScope::useException(); has no effect and I still get a fatal error. @totten @seamuslee001 do you have any ideas why?

[mfb]

When the variable goes out of scope, its destructor method reverts the error handling. If there is no reference to the object then presumably the destructor is called immediately.

I don't know why all of this is needed - seems preferable to always throw/catch exceptions so this kind of thing is predictable. I guess some legacy code relies on the fatal errors?

[seamuslee001]

@mfb more likely that no body has ever gone back to this https://issues.civicrm.org/jira/browse/CRM-11043 i think we are now slowly in the process of doing what Tim Suggested in point 4 which is to deprecate CRM_Core_Error::fatal()

[mfb]

I guess what must've tripped me up is when I tested the code, I was in a context where it did throw exceptions. But then, it wouldn't necessarily be called in that context.

[mattwire]

Ok, this fixes the issue and is ok to merge. But would be great if @totten @seamuslee001 or someone else could explain why we have to assign the return value to a variable for it to work!

[eileenmcnaughton]

I'll merge this - I don't think @mattwire's question is a blocker although it would be good to answer. I suspect the exceptions should be turned on higher up the chain here

[johncronan]

There is still one issue with this utf8mb4 check code: when you restart the server, the civicrm_utf8mb4_test table is still present, and it doesn't know to try to drop it, so the check fails.

[mfb]

There is still one issue with this utf8mb4 check code: when you restart the server, the civicrm_utf8mb4_test table is still present, and it doesn't know to try to drop it, so the check fails.

@johncronan weird - that table is dropped right away (well, unless the CREATE TABLE query failed). I wonder what is the scenario where this logic isn't working?

We could create a temporary table instead, but I wouldn't have thought this necessary.

[mfb]

The only thing I can think of off hand is if the mysql user doesn't have permission to DROP tables?

In which case.... yes we should try to drop the table first.. in case that permission was granted later and they are re-attempting the install?

[johncronan]

I was very confused how it could be possible, until I found the code in Civi/Install/Requirements.php, which does the create as a non-temporary table. I think it is a race condition caused by all the apache worker processes spinning up at the same time.

I was able to get it to stop happening by changing the CREATE and DROP statements in that file to use temporary tables also.

[mfb]

Huh... ok, I didn't think about multiple worker processes trying to install civi at the same time? Crazy :p But it seems like the logic is the same for civicrm_install_temp_table_test - wouldn't you have a problem there too?

[mfb]

Confusingly civicrm_install_temp_table_test is created as both temporary and non-temporary table...

[johncronan]

I guess that checkMysqlUtf8mb4 is run not just during install, but also on every startup. Which is great: I do appreciate this feature! Utf8mb4 stuff is a pain, so it's nice to get some advice on dealing with it.

Why it doesn't also happen with checkMysqlTrigger, I dunno. That's a good question.

[mfb]

Hmm why is it running on every startup? Can you get a stacktrace for what is calling it?

[eileenmcnaughton]

@mfb @johncronan is this causing a regression for some sites - if so can we get an issue in gitlab so we can track it as a regression?

[johncronan]

No, it's rather minor as the effect is just that the warning keeps coming back up.

[eileenmcnaughton]

@johncronan well that probably still counts as a regression - ie we prioritise dealing with regressions regardless of the seriousness because our goal is that each release should be better than the last one

[johncronan]

Oh ok. I'll see if I can get the stack trace, and open one tomorrow.