Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dev/core#3119 - Post-upgrade messages no longer being displayed #22985

Merged
merged 3 commits into from
Mar 22, 2022
Merged

dev/core#3119 - Post-upgrade messages no longer being displayed #22985

merged 3 commits into from
Mar 22, 2022

Conversation

demeritcowboy
Copy link
Contributor

Overview

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

Before

Note there isn't even the "Congratulations" message which is always present.

Untitled2

After

Untitled3

Technical Details

In #22881 (in 5.47) the session clear got moved so that it now happens before it gets to the line where it retrieves session variables telling it that it's in upgrade mode and where to find the post-upgrade messages.

Comments

Doesn't affect cv, just the UI.

This does add yet another cache clear which slows down upgrades a bit. Previous additional cache clearing had already made it take longer than it used to. But anecdotally it seems common that people need to do additional cache clears anyway.

@civibot
Copy link

civibot bot commented Mar 19, 2022

(Standard links)

@civibot civibot bot added the 5.48 label Mar 19, 2022
@demeritcowboy demeritcowboy mentioned this pull request Mar 19, 2022
Closed
@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Mar 19, 2022 •
edited
Loading

@totten can you check this - it was some work that you did here #22881

totten added 2 commits March 21, 2022 15:03
@totten
Post-upgrade messages no longer being displayed (part 2)
a181232 
This line is only hit if the upgrader activates CiviGrant.
@totten
CRM_Upgrade_Form::doFinish() - Narrower call to session-reset
efea3f5 
The main call(s) to `rebuildMenuAndCaches()` do clear out most caches.  It's
_just_ the session that has been skipped.
@totten totten force-pushed the postupgrademessage-2 branch from 792eb3a to efea3f5 Compare March 21, 2022 22:05
@totten
Copy link
Member

totten commented Mar 21, 2022

Tested on D7, upgrading from v5.45=>5.48 (with+without patch). It initially didn't work for me. With some tracing, found the problem was that #22881 had touched another call to rebuildMenuAndCaches(FALSE, $sessionReset = TRUE) (via enableExtensions()) - but this would only run if you had CiviGrant enabled (and traversed the 5.47 steps).

Pushed up a small fix for that (a181232).

(@demeritcowboy) This does add yet another cache clear which slows down upgrades a bit. Previous additional cache clearing had already made it take longer than it used to. But anecdotally it seems common that people need to do additional cache clears anyway.

(1) The doFinish() reset could be a bit narrower. As far as post-upgrade messages go, I can't see any drawbacks to this narrower formulation: efea3f5

(2) Yeah, I also noticed that (after the previous fix) it felt like the upgrade progress-bar moved slower (adding an extra/long step at the end). But I rationalized this as two things:

  • (For CiviGrant-enabling use-case) It really is adding a extra step, and that's the cost of following the normal extension lifecycle. Not worth the effort to optimize just for the upgrader.
  • (For the average use-case) It was the same number of steps, but the optics changed. Before, the final/expensive step was concurrent with an HTTP redirect/page-refresh, so it felt like the issue was with the browser loading another page. When it moved form doFinish() to doCoreFinish(), it became more apparent. But if you timed the full upgrade, I'd bet it only added 100-200ms (eg static overhead for an one AJAX call).

Anywho, I think it's good to go based on r-running - but should allow a moment in case @demeritcowboy has an feedback/reaction on a181232 + efea3f5.

Flagging merge ready.

@totten totten added the merge ready PR will be merged after a few days if there are no objections label Mar 21, 2022
@totten totten mentioned this pull request Mar 21, 2022
Merged
demeritcowboy
demeritcowboy commented Mar 22, 2022
View reviewed changes
CRM/Upgrade/Form.php
@@ -840,6 +841,8 @@ public static function doCoreFinish(): bool {
* @return bool
*/
public static function doFinish(): bool {
$session = CRM_Core_Session::singleton();
$session->reset(2);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this makes sense and is better. I didn't actually look at what config->cleanupCaches does just copied from rebuildMenuAndCaches.

demeritcowboy
demeritcowboy commented Mar 22, 2022
View reviewed changes
CRM/Upgrade/Incremental/Base.php
@@ -195,7 +195,9 @@ public static function enableExtension(CRM_Queue_TaskContext $ctx, array $keys):
// Note: A good test-scenario is to install 5.45; enable logging and CiviGrant; disable searchkit+afform; then upgrade to 5.47.
$schema = new CRM_Logging_Schema();
$schema->fixSchemaDifferences();
CRM_Core_Invoke::rebuildMenuAndCaches(FALSE, TRUE);

CRM_Core_Invoke::rebuildMenuAndCaches(FALSE, FALSE);
Copy link
Contributor Author

@demeritcowboy demeritcowboy Mar 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, civigrant - the thing this was all about.

@demeritcowboy
Copy link
Contributor Author

The test fail makes no sense, but there's been something at least a little weird with jenkins for a few days. It sounds like it's being worked on.

RM_Case_Form_EmailTest::testOpeningEmailForm
Undefined array key "valid"

/home/jenkins/bknix-dfl/build/core-22985-8w819/web/sites/all/modules/civicrm/CRM/Core/QuickForm/Action/Display.php:70
/home/jenkins/bknix-dfl/build/core-22985-8w819/web/sites/all/modules/civicrm/packages/HTML/QuickForm/Controller.php:203
/home/jenkins/bknix-dfl/build/core-22985-8w819/web/sites/all/modules/civicrm/packages/HTML/QuickForm/Page.php:103
/home/jenkins/bknix-dfl/build/core-22985-8w819/web/sites/all/modules/civicrm/CRM/Core/Controller.php:355
/home/jenkins/bknix-dfl/build/core-22985-8w819/web/sites/all/modules/civicrm/CRM/Utils/Wrapper.php:98
/home/jenkins/bknix-dfl/build/core-22985-8w819/web/sites/all/modules/civicrm/CRM/Core/Invoke.php:292
/home/jenkins/bknix-dfl/build/core-22985-8w819/web/sites/all/modules/civicrm/tests/phpunit/CRM/Case/Form/EmailTest.php:26

@totten
Copy link
Member

totten commented Mar 22, 2022

Thanks for the feedback @demeritcowboy.

Agree about the test-failure - doesn't seem to be related.

Merging.

@totten totten merged commit e365790 into civicrm:5.48 Mar 22, 2022
@demeritcowboy demeritcowboy deleted the postupgrademessage-2 branch March 27, 2022 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
5.48 merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@demeritcowboy @eileenmcnaughton @totten