Skip to content

[REF] Depends on civicrm-core#25414 Output pre-upgrade message when u…#663

Open
seamuslee001 wants to merge 1 commit into
civicrm:7.x-masterfrom
seamuslee001:pre_upgrade_message
Open

[REF] Depends on civicrm-core#25414 Output pre-upgrade message when u…#663
seamuslee001 wants to merge 1 commit into
civicrm:7.x-masterfrom
seamuslee001:pre_upgrade_message

Conversation

@seamuslee001

Copy link
Copy Markdown
Contributor

…pgrading civicrm db using drush

ping @demeritcowboy @totten

@civibot

civibot Bot commented Jan 25, 2023

Copy link
Copy Markdown

(Standard links)

@totten

totten commented Feb 1, 2023

Copy link
Copy Markdown
Member

I ran this in combination with the core PR, with an upgrade from 5.51=>master[5.59ish]. Here's how the output looked:

[bknix-min:~/bknix/build/dmaster/web/sites/all/modules/civicrm] drush cvupdb
Pre Upgrade Message:
<br />The default copies of the message templates listed below will be updated to handle 
new features or correct a problem. Your installation has customized versions of these 
message templates, and you will need to apply the updates manually after running this upgrade. 
<a href='https://docs.civicrm.org/user/en/latest/email/message-templates/#modifying-system-workflow
-message-templates' style='color:white; text-decoration:underline; font-weight:bold;' target='_blank'>
Click here</a> for detailed instructions. <ul><li><em>Contributions - Receipt (off-line)</em> - Update
 to new smarty variables for line items, tax</li></ul><p>The handling of invalid smarty template code
 in mailings, reminders and other automated messages has changed. For details, see <a href=
"https://docs.civicrm.org/sysadmin/en/latest/upgrade/version-specific/#civicrm-559" target="_blank">
upgrade notes</a>.</p>

Upgrade outputs:
WARNING: CiviCRM has changed the meaning of the date format specifier %A
when using CRM_Utils_Date::customFormat or {crmDate}. It was
identical to %P and the change will help compatibility with php 8.1.

Your Date Format [1] settings have been automatically updated.


Links:
------
[1] http://dmaster.bknix:8001/civicrm/admin/setting/date?reset=1

This is a bit incongruous in that:

  • (Small nit) "Pre-Upgrade Message" and "Upgrade Output" don't quite match. Maybe "Pre-Upgrade Messages" and "Post-Upgrade Messages"?
  • (Bigger issue) The "Pre-Upgrade Message" displays raw HTML on the console. The post-upgrade message displays more readable text (per htmlToText()).

If you look at a bit closer, the getPreUpgradeMessage() returns a string (HTML), and the run() returns an array with two formulations (['message'=>HTML, 'text'=>TEXT]).

The run() contract has been around longer, so maybe the contract for getPreUpgradeMessage() should a similar array? Then the client (drush, wp-cli, etc) can decide whether to use the HTML or TEXT version?

@totten

totten commented Feb 1, 2023

Copy link
Copy Markdown
Member

If you use civicrm/civicrm-core#25488 and the following patch, then the output will be a bit more consistent.

diff --git a/drush/civicrm.drush.inc b/drush/civicrm.drush.inc
index a63976a..145ab99 100644
--- a/drush/civicrm.drush.inc
+++ b/drush/civicrm.drush.inc
@@ -741,10 +741,10 @@ function drush_civicrm_upgrade_db() {
     return TRUE;
   }
   $upgradeHeadless = new CRM_Upgrade_Headless();
-  drush_print("Pre Upgrade Message:\n" . $upgradeHeadless->getPreUpgradeMessage());
+  drush_print("Pre-Upgrade Messages:\n" . $upgradeHeadless->getPreUpgradeMessage()['text']);
   // FIXME Exception handling?
   $result = $upgradeHeadless->run();
-  drush_print("Upgrade outputs:\n" . $result['text']);
+  drush_print("Post-Upgrade Messages:\n" . $result['text']);
 }
 
 /**

@seamuslee001

Copy link
Copy Markdown
Contributor Author

yeh ok @totten that makes sense I'll add this in tomorrow my time and we can merge this and i'll do similar for wp-cli

@totten

totten commented Feb 1, 2023

Copy link
Copy Markdown
Member

OK, sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants