[REF] Deplace usage of drupal_set_message with equivalent AddMessage …

[seamuslee001]

…function call

@KarinG @MikeyMJCO

[demeritcowboy]

Maybe a silly question but how do you even trigger this code? I was thinking maybe through drush but when I try that I get the canary message instead.

In any case I see two things code-wise:

• I probably wouldn't use addMessage since that would display the default green message. I think addWarning() or addError() makes more sense here.
• It doesn't exit after outputting this message and just continues the same as it would without the message, so was this something unfinished?

[seamuslee001]

Possibly but i would have thought it was a green message before because there is no 2nd param on the drupal_set_message

[homotechsual]

Yeah this isn't an error or warning, it's an info/success message. Ideal colour probably blue :-)

[homotechsual]

Looks like ->addStatus() is probably what we want here semantically.

[homotechsual]

Maybe a silly question but how do you even trigger this code? I was thinking maybe through drush but when I try that I get the canary message instead.

In any case I see two things code-wise:

• I probably wouldn't use addMessage since that would display the default green message. I think addWarning() or addError() makes more sense here.
• It doesn't exit after outputting this message and just continues the same as it would without the message, so was this something unfinished?

It does look like this is unfinished. Other than a nice message there's absolutely no consequence - we still proceed to trying to install even though we've just shown a message that indicates things are installed.

Missing a return? @totten git blame says this is some Tim wizardry :-)

[homotechsual]

@demeritcowboy @seamuslee001 @totten this is what it looked like pre-conversion to civicrm-setup.

https://github.com/civicrm/civicrm-drupal-8/blame/eb18bc1404d7020ae9559de4e3657eac6fc875e7/civicrm.install#L21

[demeritcowboy]

I'm going to take another run at this tomorrow. The deprecation is clear, so if I come up empty maybe we could merge and move to a lab ticket for talks about how you even get here and what should happen.

[demeritcowboy]

Ok the way to reach these lines is by using the UI or drush (but not cv) to uninstall CiviCRM, both of which don't actually uninstall any of the civi data because unlike for drupal 7 hook_uninstall doesn't seem to have been implemented. So then when you go to reinstall using drush or the UI you hit these lines.

The problem with just adding a return after the message is that drupal then still thinks it got installed fine, and civi proceeds to do a first run and finds the canary and throws an exception, and you never see the message anyway.

So I'd say change this to throw an exception, which then makes it clearer what has actually happened, and seems better for programmatic use or command line like drush. (And remove the second sentence "Skipping full installation.")

Then in future we should consider doing something with hook_uninstall and somehow avoiding even getting here because either way with an exception or message the drupal UI is left in an unusable state and the only way to get it back is by uninstalling civi with drush.

[demeritcowboy]

Added https://lab.civicrm.org/dev/drupal/-/issues/141 for the hook_uninstall.

[seamuslee001]

@MikeyMJCO Do you have any opinion based on the above? I would probably lean towards a red error myself but maybe an exception might be appropriate maybe?

[demeritcowboy]

What I'm saying though is that you never see that error, because even with or without adding a return after it drupal still continues on and you see a different exception which doesn't tell you what actually happened.

[seamuslee001]

Closing in favour of #53