Fix layout styles after run migration

[yurabakhtin]

What kind of change does this PR introduce? (check at least one)

• Bugfix

The PR fulfills these requirements:

• It's submitted to the develop branch, not the master branch if no hotfix
• All tests are passing
• Changelog was modified

Other information:
Issue: #6711 (comment)

[martin-rueegg]

@yurabakhtin nice catch! Thank you for having investigated this further and being able to find the reason. I guess the result is an elegant solution.

The comments in regards to specific lines are just some thoughts that came when reviewing the code, that may be worth reflecting upon. But no reason for not merging this change.

[martin-rueegg]

I wonder if it is better or worse, if a specific DB_ACTION_RESULTS should be used, rather than relying on the existence of the key in the session.

[yurabakhtin]

@martin-rueegg Sorry, not clear what you mean here.
Do you want to use the session key DB_ACTION_RESULTS instead of my DB_MIGRATION_RESULT_KEY?
Or do you want to store the migration result in some other place instead of Sessions?

[martin-rueegg]

@yurabakhtin please accept my apologies if I have not been clear enough.

\humhub\modules\admin\controllers\InformationController::actionDatabase() takes an optional argument, int $migrate, which may currently take one of the following values:

• public const DB_ACTION_CHECK = 0; (default)
• public const DB_ACTION_RUN = 1;
• public const DB_ACTION_PENDING = 2;

With your PR, the default case will be further split in the process based on the fact wheter or not there is a non-null value stored in the session's DB_MIGRATION_RESULT_KEY.

I personally would not rely on the fact whether a DB_MIGRATION_RESULT_KEY is stored or not in the session, but I would introduce a fourth case for the above argument, e.g.

• public const DB_ACTION_RESULT = 3;

and then add that argument to the redirection:

return $this->redirect(['/admin/information/database', 'migrate' => self::DB_ACTION_RESULT]]);

Then, following the redirection I would

if ($migrate === self::DB_ACTION_RESULT ) {
    $migrationOutput = Yii::$app->session->get(self::DB_MIGRATION_RESULT_KEY);

    // in case something went wrong
    $migrationOutput ??= "Migration result could not be retrieved from session";

    Yii::$app->session->remove(self::DB_MIGRATION_RESULT_KEY);
} else {
    $migrate = $migrationService->hasMigrationsPending()
        ? self::DB_ACTION_PENDING
        : self::DB_ACTION_CHECK;
    $migrationOutput = $migrationService->getLastMigrationOutput();
}

I find that would be a more clear structure and also the web server access log would differentiate between the two cases, which may be helpful during diagnose of any issue.

This would also address the case discussed below regarding Yii::$app->session->get() vs. Yii::$app->session->has();

I hope this makes more sense now! :-)

[yurabakhtin]

@martin-rueegg Thank you for the detailed answer.
I again reviewed the code and decided to refactor it because I think we should not keep so much logic in controller, so I have moved the migration process into the MigrationService, I hope now the code looks better and more readable, please review that, thanks.
Commit 84acc15.

[martin-rueegg]

Hi @yurabakhtin

Thanks for the update.

I think moving some code to MigrationService is a valid approach. I'm less sure about moving the flushCache method to the cache settings form model. I'd rather use a CacheHelper or CacheService. Maybe check with @luke- here?

As far as the changes are regarded: strictly speaking you're breaking backward-compatiblity by moving the flushCache method and the public constants. May not be an issue but should at least be mentioned in the MIGRATE.md.

Also, the whole point about using an additional parameter in the redirect, as showed above, is ignored.

Hope that helps as a feedback.

[yurabakhtin]

@martin-rueegg

I'm less sure about moving the flushCache method to the cache settings form model. I'd rather use a CacheHelper or CacheService

I have moved the flushCache from SettingController to CacheSettingsForm because I think better to keep the method inside the model form because firstly it is started to use after save the form.
Yes, of course better to keep such code in some Helper or Service but I didn't find such,

As far as the changes are regarded: strictly speaking you're breaking backward-compatiblity by moving the flushCache method and the public constants. May not be an issue but should at least be mentioned in the MIGRATE.md.

I see the method flushCache has @since 1.16 and the contants were added only in develop-1.16, i.e. they were not released yet, so we don't need care about this, or am I wrong?

Also, the whole point about using an additional parameter in the redirect, as showed above, is ignored.

I don't see a reason to keep the the session param in url, i.e. what reason to display ?migrate=3 for user/admin? I think it will be confused. I specially to make the param hidden because user/admin should not think about this. The DB_ACTION_SESSION = 3 was created only to fix the issue what we have after flush cache/assets.
I mean after successful migration admin should see in address bar only url like /information/database, admin should not see there url like /information/database?migrate=3 because after resfresh such url is not clear what to do, i.e. such url does the same what url /information/database does when no data stored in session from previous migration process. So the url /information/database?migrate=3 will be only confused I think.

[martin-rueegg]

Could there be an other reason for the variable to be null? Could it be done with something like Yii::$app->session->has() (if such a method exists)?

[yurabakhtin]

yes, the method Yii::$app->session->has() exists, but even if we call it then we must call Yii::$app->session->get() in order to have $migrationOutput because then it is passed into the view file.
So new code may be like this:

if (Yii::$app->session->has(self::DB_MIGRATION_RESULT_KEY)) {
    $migrationOutput = Yii::$app->session->get(self::DB_MIGRATION_RESULT_KEY);
    Yii::$app->session->remove(self::DB_MIGRATION_RESULT_KEY);
}

instead of current:

$migrationOutput = Yii::$app->session->get(self::DB_MIGRATION_RESULT_KEY);
if ($migrationOutput !== null) {
    Yii::$app->session->remove(self::DB_MIGRATION_RESULT_KEY);
}

I don't see a reason to call here the Yii::$app->session->has() because it does almost same as Yii::$app->session->get().

[what-the-diff]

PR Summary

• Layout Fix in Developer Changelog
  The developer-oriented changelog file (CHANGELOG-DEV.md) went through a layout improvement following a migration process.

• Database Migration Enhancements
  Several modifications were made in the InformationController.php aiming to improve database migrations. Changes include using more standardized constants from a specifically designed service (MigrationService), and redirecting the page to show database information when the migration is in a specific state. Also, the way migration status gets displayed on database.php was refined.

• Caching Mechanism Updates
  In the SettingController.php and CacheSettingsForm.php files, adjustments were made to how the system handles cache clearing. The flushCache() method was relocated to a new class, resulting in a more structured and organized approach. Furthermore, after saving cache settings, the cache will now be cleared automatically, enhancing system performance and efficiency.

• Migration Service Improvements
  The MigrationService.php file underwent several upgrades geared towards handling database migrations more efficiently. New functionality was added that allows it to run migration actions more dynamically and store and retrieve the last migration output using sessions, providing a more streamlined experience during migrations.