Fix unreplaced template vars in civicrm.settings.php when using legacy installer

[demeritcowboy]

Overview

When using the legacy installer (e.g. sites/all/modules/civicrm/install/index.php) it leaves behind some unreplaced ssl-related template variables.

Before

DSN in civicrm.settings.php looks like mysql://foo:bar@localhost/cividb?new_link=true%%dbSSL%% after install.

After

mysql://foo:bar@localhost/cividb?new_link=true

Technical Details

The ssl vars are only replaced when using the newer civicrm-setup.

Comments

See also civicrm/civicrm-joomla#63

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

[demeritcowboy]

Jenkins retest this please.

PhantomJS 2.1.1 (Linux x86_64) ERROR
Disconnected , because no message in 30000 ms.

[totten]

This looks safe and reasonable on its own.

Not a blocker, but possibly related - I don't see dbSSL in civicrm.drush.inc for BD/D7/D8?

[demeritcowboy]

Oh that probably has the same problem then. And wp-cli if it does installs too.

[demeritcowboy]

Have added #21710 as alternate but for readability I think I'd prefer this one and just try to find all the spots where it's used, but the upgrade message might be useful either way.

[totten]

Yeah, I think they're complementary. #21692 looks like a fix for new installations, and #21710 looks like cleanup for existing installations.

[demeritcowboy]

I meant I would move the upgrade message here and close that one if this is the more desirable route.

[eileenmcnaughton]

@totten @demeritcowboy this looks almost there but to have lost momentum ...

[demeritcowboy]

If the upgrade message is a holdup I can remove it. The actual change is just the blanks. There's similar PRs for drush at civicrm/civicrm-drupal-8#67, civicrm/civicrm-drupal#650, civicrm/civicrm-backdrop#151, and the wp-cli and joomla ones are merged.

[eileenmcnaughton]

@demeritcowboy so I'm confused what this is blocked on - my take is that when we weren't swapping out the ssl variables in our civicrm.settings.php we couldn't load the site - but if you think this is a misconfiguration that might be in operation then I think the message is OK for the edge case

[demeritcowboy]

It's cosmetic at the moment because the %% stuff gets ignored anyway so civi still works fine. But strictly speaking it shouldn't leave the %% variables in there. There's a couple approaches:

• Just blanking them - the simplest
• Supporting db ssl connections for the legacy installer. I don't think it's worth doing.
• Something fancier which was the alternate PR, but I felt it was too unreadable and all it gained was avoiding having to make those other drush etc PRs which aren't that hard to do.

The upgrade message isn't an edge case. It's anyone who used the legacy installer or drush since FiveTwentySomething would likely have the %% vars still in their civicrm.settings.php.

[eileenmcnaughton]

@demeritcowboy ok - but those hitting the upgrade message - it's an optional thing for them to fix?

[demeritcowboy]

At the moment yes. Theoretically it could cause problems in the future if there was some change to DSN parsing or usage. I can tone down the message?

[eileenmcnaughton]

@demeritcowboy it feels like it should be a status check IMHO. I'm inclined to say pull it out of this PR & we'll merge this one & then the other PR can deal with informing people

[demeritcowboy]

Ok I'll pull it out. I don't know if it should be a status check because the person who would deal with it is often not the person who sees the status check message. Maybe it could be communicated some other way TBD.

[eileenmcnaughton]

unrelated fail