Upgrading a site that still has "mysql" in the dsn string breaks in latest master

I think it's partly because of https://github.com/civicrm/civicrm-packages/pull/302 but more because I remember once seeing something somewhere that civi silently converted mysql to mysqli for you, and maybe that has also been removed somewhere. While reviewing it maybe I missed it.

The fix is easy, just update your DSN in civicrm.settings.php to explicitly say "mysqli".

The technical issue is that without that silent conversion, it ends up trying to load php-mysql. It's just confusing at first because the error is extension not found, which makes you think "Civi Extension", but it means php extension.

Going to label it regression, but it's not the usual type of regression and it just started yesterday.

added regression label

https://github.com/civicrm/civicrm-packages/pull/302/files#diff-f6dbd6df5a9778c95042bebe33ebeeb4L758

So this is interesting. The patch file has it, but it didn't seem to get applied. Maybe the filepath for the file to update is wrong? Would it give an error?

https://raw.githubusercontent.com/civicrm/civicrm-core/a48a43c2b5f6d694fff1cfb99d522c5d9e2459a0/tools/scripts/composer/pear_db_civicrm_changes.patch

https://github.com/civicrm/civicrm-core/commit/b687e8d7f9f035313febbdd4b9e5cbd19545f9f1#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780R256

It occurs to me I should mention this was drupal 8. Maybe the order things happen via composer is different.

Even more interesting. It seems this site did not have "enable-patching": true in the top-level composer.json. It's a fairly stock site, initially created July 3. Curious what other people have and if civi patches are being applied there.

If I add enable-patching and then run composer install it starts applying the patches.

Even more interesting is how come nothing else has been failing without all those patches. Or maybe it does it on install but then not update.

@DaveD yeh the patching thing may not be on everyone's radar that might be useful for the install docs to mention that for best results. I suspect what it has meant is that various (non really breaking) patches have been missed. So this is the patch here that was probably the issue https://github.com/civicrm/civicrm-packages/pull/302/files#diff-f6dbd6df5a9778c95042bebe33ebeeb4L758. I also wonder if we should actually bring that into core itself (as well as possibly being in the package)

@DaveD I have added a PR to update the Drupal 8 installation documentation documentation/docs/installation!6 (merged) pushing for patching to be enabled

Thanks will take a look. I haven't double-checked what an initial install without it does but either way yes I think it's desired anyway.

I think the fact the civicrm.settings.php.template also this function https://github.com/civicrm/civicrm-core/blob/master/setup/src/Setup/DbUtil.php#L28 prefix with mysql suggests its a potentially live issue

Actually it's funny I just looked at that function 30 minutes ago for something else and can't see where it's used, but your point seems right.

mentioned in merge request documentation/docs/installation!6 (merged)

changed milestone to %5.30.0

Just checked - a new install (pure composer, not buildkit) without the setting does NOT apply the patches. So it's not just when updating a site.

Closing since docs PR is merged.

closed

reopened

Reopening since people upgrading aren't going to read the installation page. Maybe needs a preupgrade message.

Oh, but where would the upgrade message go since you won't be able to load the upgrade page.

@DaveD we could put it in the previous version's checks? Or @andrewhunt @totten could ensure it is highlighted in the release blog?

Oh - it will affect MOST sites! We need to do more - I think we need to handle mysqli - even if hacking packages - we can do so noisily...

It could go in the 5.28/5.29 upgrade messages yes. Just if they skip those releases and go to 5.30+ then they won't even get to any upgrade message before crashing.

Civi could handle mysqli somewhere in core, and then it would be for the purpose of allowing getting to the upgrade message to tell them what the actual problem is which is the patching.

Hacking packages won't work because it won't apply the hack on the updated site for the same reason as the problem itself. Maybe something like a stub DB.php which removes itself via patching if patching is working, but that's getting weird.

Another thought: if civi could really early on before accessing the civi database, e.g. in the drupal-8 module maybe, give a nice message if it appears it didn't get patched. To double-check: does cv go through the module?

For other CMSs the problem would be an autoloader issue, but it only comes up if they manage their sites via git pull + composer and don't pull packages before running composer. So this seems rare.

Also that is only if they are managing using a git repository based on our civicrm-core repo, for example my former employer has their own git repo but that is much more based on the Drupal7 tarball rather than the civicrm-core repository. In that case they would be fine because the tar process will ensure that the patch is applied correctly