humhub / updater

Upgrade your HumHub installation to the latest version in a few clicks.
8 stars 12 forks source link

Merge to HumHub core? #45

Open marc-farre opened 6 months ago

marc-farre commented 6 months ago

I think it's not obvious that updating HumHub core requires installing a module.

In other similar softwares, the updater is part of the core. On Nextcloud, it's even the landing page of the administration settings (it shows the core version, if there are updates and warnings regarding the setup): image

I can see some sys admin not updating their HumHub instance because they are not aware of this module (which is not easy to find among the long list of existing modules in the Marketplace).

If this module was included into core, sys admin would automatically receive notifications when a new version is available.

luke- commented 6 months ago

@marc-farre Good point, In principle, I am also in favor of including this module in the core.

The only advantage I see with the module version is that we are a bit more flexible and can quickly publish a new Updater module version if necessary.

But let's merge!

dreua commented 6 months ago

Maybe you can have both? The arguments that @marc-farre describes are all UI/UX related if I understand correctly: Make the admin landing page recognize whether the updater is installed and link to the module installation if it isn't. Once it is installed the page can proceed to notify for updates.

That's just an idea, do whatever seems best and easiest to maintain ;)

marc-farre commented 6 months ago

@dreua I mentioned Nextcloud only to show the importance of having the Updater module included in the core. But this issue is only about merging this module in HumHub core: https://github.com/humhub/humhub/ It won't change anything in term of UI/UX, the Updater will remain accessible from the Administration left menu.

If you think it's better to display available updates on the Administration landing page, you can open a dedicated issue on https://github.com/humhub/humhub/issues/

marc-farre commented 6 months ago

@luke- I can create 2 PRs

On the core repository:

On this repository:

I'm not quite sure about this last step. I was thinking something like this:

public function actionInstallFiles()
    {
        /* @var Module $module */
        $module = Yii::$app->getModule('updater');
        $oldModulePath = $module->getBasePath();

        $this->updatePackage->install();

        if (version_compare(Yii::$app->version, '1.16', '>=')) {
            \yii\helpers\FileHelper::removeDirectory($oldModulePath);
            $this->flushCaches();
        }

        if (function_exists('opcache_reset')) {
            @opcache_reset();
        }

        return ['status' => 'ok'];
    }
luke- commented 6 months ago

Sounds good.

We should definitely make sure that the ModulManager, from 1.18, no longer loads the updater from the module folder.

Maybe we can then do a cleanup via the core migrations?

marc-farre commented 6 months ago

ModulManager, from 1.18, no longer loads the updater

You mean from HumHub 1.16?

Do you think public bool $preventDuplicatedModules = true; (https://github.com/humhub/humhub/blob/master/protected/humhub/components/ModuleManager.php#L94) is not enough?

cleanup via the core migrations

Yes, you're right, in the case of manual updates.

But we also need the \yii\helpers\FileHelper::removeDirectory($oldModulePath); I suggested adding to InstallController::actionInstallFiles() (see above) otherwise we will have a bug on InstallController::actionMigrate() if we don't, as at this stage the module will be duplicated.

luke- commented 6 months ago

ModulManager, from 1.18, no longer loads the updater

You mean from HumHub 1.16?

Yes, I would include the Upgrader module in either version 1.16 or 1.17. Depending on how reliable the solution is.

Do you think public bool $preventDuplicatedModules = true; (https://github.com/humhub/humhub/blob/master/protected/humhub/components/ModuleManager.php#L94) is not enough?

Not sure, need to look into the code. Ideally, there are no fatal errors or warnings when the "Updater" module exists twice for a while. In addition, the core module should always be loaded.

cleanup via the core migrations

Yes, you're right, in the case of manual updates.

But we also need the \yii\helpers\FileHelper::removeDirectory($oldModulePath); I suggested adding to InstallController::actionInstallFiles() (see above) otherwise we will have a bug on InstallController::actionMigrate() if we don't, as at this stage the module will be duplicated.

Ideally there is no big error, when this module exists twice. But maybe it's also a solution to delete it via updater.


I still have to think about the disadvantages of the integrated updater.

For example, we can currently simply replace the updater for all old versions, e.g. to cover special cases. This will then no longer be possible.

marc-farre commented 6 months ago

Another option would be to install it by default when installing HumHub and, if the HumHub version is outdated but the Updater module is not installed, we display a warning somewhere with a link to install the module.

luke- commented 6 months ago

Yes, the solution currently seems more suitable to me.

A separate installer step "Install Updater Module"