dev/core#1937 - Upgrade message about needing composer patching turned on and updating mysql in DSN strings

[demeritcowboy]

Overview

https://lab.civicrm.org/dev/core/-/issues/1937

In drupal 8 you need composer's enable-patching to be turned on for civi patches to get applied.

Technical Details

In particular for 5.30 because pear::DB has moved, the patch that would get applied to handle automatically using mysqli instead of mysql in the dsn string won't get applied, so it crashes. Most sites likely still have mysql in their civicrm.settings.php dsn strings.

But there's other patches for civi needed too even pre-5.30, you just don't get a crash without them.

Comments

There's still more needed because you won't even be able to get to this upgrade message if you go straight from your current version to 5.30 and don't try to install a pre-5.30 version first.

[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).

[eileenmcnaughton]

@demeritcowboy can we ALSO check for their dsn having mysqli & notify all sites with that?

[eileenmcnaughton]

The good thing here is that anyone doing upgrades by composer should have some basic knowledge - I think we should add a simple blog to point people to

[demeritcowboy]

I can add the mysqli part but that's not the actual problem here it's just one symptom. I can do that as a separate message for all cms's.

Based on the drupal chat channel being filled with composer questions I'm not sure basic knowledge is a given. There's probably a lot of people who aren't interested in composer and are only using it because there's no tarball.

[eileenmcnaughton]

@demeritcowboy ok - that makes sense - I think we want somewhere to point people to though - I like a blog because it has visibility - but it could be a gitlab. I'll add to the email I'm preparing to send to the dev list

[eileenmcnaughton]

Oh I see - you DO have a link - seems good to me then - @seamuslee001

[demeritcowboy]

Oops I have a typo. One minute...

[demeritcowboy]

I had copied the url for the docs link from somewhere and I'm not sure if redirection has changed on the docs server or if the other one was wrong too. I don't want to link to the "en" version directly so I'm not sure what the right url is.

[demeritcowboy]

Arrggh -

civicrm-core/CRM/Utils/System.php

Line 1419 in dd76c9c

return preg_replace('#^(user|sysadmin|dev)/#', '\1/en/latest/', $url);

Doesn't handle installation doclinks.

[totten]

I think the idea of an automatic message about composer patching is a really good one, and I completely agree that knowledge of enable-patching isn't a given (even among D8 folks). But... I'd like to suggest a better place to put the automatic check.

TLDR: Create a composer package/plugin whose sole purpose is to complain vociferously if the root package doesn't have enable-patching. (ex: civicrm/require-composer-patches) Add that as a dependency for civicrm-core.

Rationales:

• As you point out, the DSN issue is just one symptom of missing patches. As long composer's default is to ignore patches, and as long as CiviCRM genuinely requires patches, and as long as it's only tested with patches... it'll be a recurring problem (just chasing different symptoms based on time/version/use-case).
• The problem of missing patches is just as likely to affect new deployments as upgrade deployments.
• IMHO, it's preferable to support DSN's with the mysql:// notation - on account of the install-base, downstream+upstream installer scripts, and general aesthetics. Tying Civi's stored-configurations to mysqli:// means it'll break again whenever php.net gets tired of maintaining mysqli and PDO-MySQL concurrently.
• As far as composer plugins go, it would be fairly small one.

[demeritcowboy]

I removed the mysql part but left in the pre-announcement about patches.

[eileenmcnaughton]

I think this looks OK - & there might be some follow ons...