Check module requirements before install/update reading the new requirements.php file in modules #6744

luke- · 2023-12-15T17:02:15Z

          I would have expected `Module::update()` here. Do you have an idea?

humhub/protected/humhub/modules/marketplace/components/OnlineModuleManager.php

Line 176 in 374501b

$updatedModule->getMigrationService()->migrateUp();

Originally posted by @luke- in #6550 (comment)

The text was updated successfully, but these errors were encountered:

martin-rueegg · 2023-12-15T17:18:10Z

Improve Module Update #6687

marc-farre · 2024-01-13T06:26:52Z

Duplicate of #4653 (I close the old one).

marc-farre · 2024-01-13T06:35:54Z

@luke- Do you think we can make work, for Humhub 1.15.3, the Module::beforeUpdate() to cancel module updating if returning false (tested, it's not working, even on 1.16)?
See https://github.com/humhub/humhub/blob/master/protected/humhub/components/Module.php#L329-L339

This would allow me to stop updating modules if the server is still on PHP 7.4, because of the issue #6797, since there are some modules that cannot be reverted for PHP 7.4 (the Web Feed module and the eCommerce module), because it would be much too much work to downgrade them (I use new external libraries and new PHP 8 syntaxes such as int|string $var or $var?->method() everywhere in the code).

If I can help on this issue, don't hesitate. Thanks!

…dule update

luke- · 2024-01-15T12:17:52Z

@marc-farre Would this solves the issue?
https://github.com/humhub/humhub/pull/6799/files

You can then overwrite the update() and throw an expection when PHP Version is too low.

marc-farre · 2024-01-16T07:32:37Z

@luke- Thanks, that's great!

However, it cannot work in this use case, as even if I throw an exception here, the module is already updated:

humhub/protected/humhub/modules/marketplace/components/OnlineModuleManager.php

Line 176 in ee95fde

$updatedModule->update();

The problem is that we need first to remove the old module and install the new one to get the code of the new module which checks PHP version. But then it's too late. The only possibility I see would be to restore the backup to reinstall the old one.
If think it adds too much complexity and that we should leave it for now.

Anyway, this issue is solved I think with your PR and that will allow us now to perform some actions on module update, that's a good point!

luke- · 2024-01-16T10:16:56Z

@marc-farre Ok, understand. Indeed such an "beforeUpdate" is really tricky to implement. We recently had a similar discussion with Martin. I hope we can find a simple solution for this in the future.

martin-rueegg · 2024-01-17T11:05:22Z

@marc-farre Ok, understand. Indeed such an "beforeUpdate" is really tricky to implement. We recently had a similar discussion with Martin. I hope we can find a simple solution for this in the future.

I think in the module update process, there should be implemented a new step "prepare upgrade" or maybe the module class should implement a function isUpdatePrevented(string $newVersion): ?string. Then, the OnlineModuleManager (OMM) should call that method when displaying the modules that have a newer version available online. If the method returns null, the upgrade is possible. Otherwise the method returns a reason that can be displayed to the user as to why the update is not possible. If multiple new versions are available online, the OMM would check all the available versions.

Or something along these lines. The actual upgrade would then happen in a separate request.

Happy to make a suggestion in a PR for #6687. I have already worked on a suggestion, but that is blocked by #6745 which has not yet been merged.

marc-farre · 2024-01-24T07:50:24Z

@martin-rueegg I agree. This would be very nice to have.

We already have a check in the Online Marketplace for the minimal Humhub version in the module.json file.

Maybe we could either add more checks here, such as the PHP minimal version and required PHP extensions, or as you suggest a call to a PHP function of the Module.php file which should return true or false.

luke- · 2024-01-24T09:10:21Z

A simple solution could be to allow a check via an anonymous function in the config.php file.
Before a update installation/update, we could first extract the single file config.php and run the method.

return [
    'id' => 'saml-sso',
    'class' => 'humhub\modules\sso\saml\Module',
    'requirementCheck' => function (): ?string {
        if (version_compare(PHP_VERSION, '8.0', '<')) {
            return 'Your PHP Version is too old!';
        }

        return null;
    },
    //...
];

marc-farre · 2024-01-24T09:14:23Z

I think that would be perfect!
This way, we can also check all required PHP extensions.
Should I create an issue for this idea?

martin-rueegg · 2024-01-24T10:28:41Z

A simple solution could be to allow a check via an anonymous function in the config.php file. Before a update installation/update, we could first extract the single file config.php and run the method.
return [
    'id' => 'saml-sso',
    'class' => 'humhub\modules\sso\saml\Module',
    'requirementCheck' => function (): ?string {
        if (version_compare(PHP_VERSION, '8.0', '<')) {
            return 'Your PHP Version is too old!';
        }

        return null;
    },
    //...
]; 

Please note, that the config is cached and I'm not sure the function would be serialized correctly in every possible circumstance (depending on the caching backend).

Why not using a well-known event instead? That would also allow other modules to intercept an update of a module it depends on.

It could be something along the lines of input validation of a form/model rules. In the sense, that the event (e.g. ModuleUpdateCheck) would provide and empty array that can be filled with error messages in case an update should not take place. If the array remains empty, the update is allowed, if there are errors, they can be displayed.

martin-rueegg · 2024-01-24T10:32:04Z

class ModuleUpdateCheckEvent {
public string $moduleId;
public array $moduleConfig; // from config.php
public array $moduleInfo; // from module.json
public array $errors = [];
}

Or something along those lines?

luke- · 2024-01-24T11:05:46Z

I'd like to keep it as simple and as compact as possible.

Using config.php only a few lines would be necessary. Here is an untested example:

https://github.com/humhub/humhub/pull/6816/files#diff-2c845135215bd46d0d95bf3dbfb3271c447870fc89acdbe8faed9abc604e12baR104-R112

marc-farre · 2024-01-24T17:27:07Z

@luke- thanks very much!

If you want me to test, should I push a "Test" module on https://partner.humhub.com/ which I will delete after testing? Another idea?

marc-farre · 2024-01-25T06:18:44Z

@luke- It's OK for testing: I must release a new version of the eCommerce module today. I will already include the new requirementCheck into the config.php file, which will allow me to test your work.

luke- · 2024-01-25T09:04:31Z

@marc-farre Thank you for your help with testing.

I have just sent you details of our test environment. You are welcome to publish a test module there.

marc-farre · 2024-01-25T20:54:10Z

@luke- I've just tested installing the test module with PHP 7.4 (the module requires 8.0).
It works well. Maybe we could have a better danger flash box that writes directly the message, because few people will know how to see the error message:

Now, when I upgrade my server to PHP 8, I can install the module, but when I click on "Activate now", I have this message:

I have this error message on all modules.

But I can then activate them on the module manager (Administration -> Module), it works well:

Must be something to do with the test environment.

The problem, is that I cannot test updating, because in the Marketplace page, it doesn't detects that my module is already installed, I still have the "install" button (but I don't have this problem with other modules, so maybe something is missing in my test module):

Something else more problematic:

If I try to install the test module on the master branch (because if we send a module with the requirementCheck in the config.php file it might be installed in older Humhub versions), just after clicking on "Enable" I have:

2024-01-25 20:32:20 [::1][1][-][error][Exception] Exception: Serialization of 'Closure' is not allowed in /var/www/hhtest/protected/vendor/yiisoft/yii2/caching/Cache.php:249
Stack trace:
#0 /var/www/hhtest/protected/vendor/yiisoft/yii2/caching/Cache.php(249): serialize()
#1 /var/www/hhtest/protected/humhub/components/bootstrap/ModuleAutoLoader.php(49): yii\caching\Cache->set()
#2 /var/www/hhtest/protected/humhub/components/bootstrap/ModuleAutoLoader.php(34): humhub\components\bootstrap\ModuleAutoLoader::locateModules()
#3 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/Application.php(325): humhub\components\bootstrap\ModuleAutoLoader->bootstrap()
#4 /var/www/hhtest/protected/vendor/yiisoft/yii2/web/Application.php(69): yii\base\Application->bootstrap()
#5 /var/www/hhtest/protected/humhub/components/Application.php(61): yii\web\Application->bootstrap()
#6 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/Application.php(271): humhub\components\Application->bootstrap()
#7 /var/www/hhtest/protected/humhub/components/Application.php(42): yii\base\Application->init()
#8 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/BaseObject.php(109): humhub\components\Application->init()
#9 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/Application.php(204): yii\base\BaseObject->__construct()
#10 /var/www/hhtest/protected/humhub/components/ApplicationTrait.php(38): yii\base\Application->__construct()
#11 /var/www/hhtest/index.php(25): humhub\components\Application->__construct()
#12 {main}

And then, I always have this (the Humhub is crashed, I cannot use it anymore, unless I switch again on the enh/check-module-requirements branch):

An Error occurred while handling another error:
Error: Call to a member function get() on null in /var/www/hhtest/protected/humhub/modules/file/validators/FileValidator.php:46
Stack trace:
#0 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/BaseObject.php(109): humhub\modules\file\validators\FileValidator->init()
#1 /var/www/hhtest/protected/humhub/widgets/CoreJsConfig.php(65): yii\base\BaseObject->__construct()
#2 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/Widget.php(146): humhub\widgets\CoreJsConfig->run()
#3 /var/www/hhtest/protected/humhub/modules/ui/view/components/View.php(491): yii\base\Widget::widget()
#4 /var/www/hhtest/protected/vendor/yiisoft/yii2/views/errorHandler/exception.php(538): humhub\modules\ui\view\components\View->endBody()
#5 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/View.php(347): require('/var/www/hhtest...')
#6 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/View.php(257): yii\base\View->renderPhpFile()
#7 /var/www/hhtest/protected/vendor/yiisoft/yii2/web/ErrorHandler.php(270): yii\base\View->renderFile()
#8 /var/www/hhtest/protected/vendor/yiisoft/yii2/web/ErrorHandler.php(127): yii\web\ErrorHandler->renderFile()
#9 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/ErrorHandler.php(152): yii\web\ErrorHandler->renderException()
#10 [internal function]: yii\base\ErrorHandler->handleException()
#11 {main}
Previous exception:
Exception: Serialization of 'Closure' is not allowed in /var/www/hhtest/protected/vendor/yiisoft/yii2/caching/Cache.php:249
Stack trace:
#0 /var/www/hhtest/protected/vendor/yiisoft/yii2/caching/Cache.php(249): serialize()
#1 /var/www/hhtest/protected/humhub/components/bootstrap/ModuleAutoLoader.php(49): yii\caching\Cache->set()
#2 /var/www/hhtest/protected/humhub/components/bootstrap/ModuleAutoLoader.php(34): humhub\components\bootstrap\ModuleAutoLoader::locateModules()
#3 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/Application.php(325): humhub\components\bootstrap\ModuleAutoLoader->bootstrap()
#4 /var/www/hhtest/protected/vendor/yiisoft/yii2/web/Application.php(69): yii\base\Application->bootstrap()
#5 /var/www/hhtest/protected/humhub/components/Application.php(61): yii\web\Application->bootstrap()
#6 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/Application.php(271): humhub\components\Application->bootstrap()
#7 /var/www/hhtest/protected/humhub/components/Application.php(42): yii\base\Application->init()
#8 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/BaseObject.php(109): humhub\components\Application->init()
#9 /var/www/hhtest/protected/vendor/yiisoft/yii2/base/Application.php(204): yii\base\BaseObject->__construct()
#10 /var/www/hhtest/protected/humhub/components/ApplicationTrait.php(38): yii\base\Application->__construct()
#11 /var/www/hhtest/index.php(25): humhub\components\Application->__construct()
#12 {main}

luke- · 2024-01-26T09:51:11Z

@marc-farre Thanks for testing. The handler not found errors seems to be related with the test environment.

Probably an anonymous function in the module "config.php" is not so ideal after all, as we serialize & cache this files. We then always need to clean up the file before caching.

Another solution could be to allow a module to come with an e.g. requirements.php file in the root folder. This could contain a check e.g.

<?php

if (version_compare(PHP_VERSION, '8.0', '<')) {
    return 'Your PHP Version is too high!';
}

return null;

If we move the check from the "OnlineModuleService" class to "ModuleService", we should be able to display the error message relatively well in the UI.

Unfortunately, no translations are possible here, as only the single file of the new module package is executed and no translations etc. of the new module version are available here.

But basically I like the solution with a single file like "requirements.php", because it only applies to very few modules that have special system requirements that differ from the HumHub core.

marc-farre · 2024-02-01T11:08:56Z

@luke- the new requirements.php is fine.

The error is well displayed:

But the module is uninstalled. The error should trigger before uninstalling it. I can have a look to the code in a few days if you want.

luke- · 2024-02-01T14:11:24Z

@marc-farre Thanks for testing. Strange, the requirement check should be executed before uninstall here. https://github.com/humhub/humhub/pull/6816/files#diff-2c845135215bd46d0d95bf3dbfb3271c447870fc89acdbe8faed9abc604e12baR200 Would be great if you have some time to have a look.

marc-farre · 2024-02-01T21:16:39Z

@luke- Fixed in commit 0336a3d
Now it seems to be fine, I think you can merge to develop. Thanks again for your work!

I've tested:

new module install
module update
module update with requirement failed and success

marc-farre · 2024-02-02T07:17:41Z

@luke- what do you think about this commit? fb673ab

It allows adding the error to the log.
It would be great for command line, especially if using module/update-all in a cron job.

marc-farre mentioned this issue Jan 13, 2024

Module::update() is not called after module update with the marketplace #4653

Closed

luke- added a commit that referenced this issue Jan 15, 2024

Fix #6744: Make sure to call humhub\components\Module::update() on Mo…

ee95fde

…dule update

luke- closed this as completed in c5322bd Jan 16, 2024

martin-rueegg mentioned this issue Jan 17, 2024

Improve Module Update #6687

Open

marc-farre reopened this Jan 25, 2024

marc-farre changed the title ~~OnlineMarketplace class should call Module::update()~~ Check module requirements before install/update reading the new requirements.php file in modules Feb 2, 2024

marc-farre mentioned this issue Feb 2, 2024

Display module requirements on the admin Prerequisites page #6831

Open

marc-farre mentioned this issue Feb 2, 2024

Add the new requirements.php file in modules having specific requirements #6832

Closed

luke- closed this as completed Feb 2, 2024

This was referenced Feb 28, 2024

Add requirements.php humhub/linkpreview-issues#41

Closed

Add requirements.php humhub/rest#155

Closed

Add requirements.php humhub-contrib/external_calendar#69

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check module requirements before install/update reading the new requirements.php file in modules #6744

Check module requirements before install/update reading the new requirements.php file in modules #6744

luke- commented Dec 15, 2023 •

edited

martin-rueegg commented Dec 15, 2023

marc-farre commented Jan 13, 2024

marc-farre commented Jan 13, 2024 •

edited

luke- commented Jan 15, 2024

marc-farre commented Jan 16, 2024

luke- commented Jan 16, 2024

martin-rueegg commented Jan 17, 2024

marc-farre commented Jan 24, 2024

luke- commented Jan 24, 2024

marc-farre commented Jan 24, 2024

martin-rueegg commented Jan 24, 2024

martin-rueegg commented Jan 24, 2024

luke- commented Jan 24, 2024

marc-farre commented Jan 24, 2024

marc-farre commented Jan 25, 2024

luke- commented Jan 25, 2024

marc-farre commented Jan 25, 2024

luke- commented Jan 26, 2024

marc-farre commented Feb 1, 2024

luke- commented Feb 1, 2024

marc-farre commented Feb 1, 2024

marc-farre commented Feb 2, 2024

Check module requirements before install/update reading the new requirements.php file in modules #6744

Check module requirements before install/update reading the new requirements.php file in modules #6744

Comments

luke- commented Dec 15, 2023 • edited

martin-rueegg commented Dec 15, 2023

marc-farre commented Jan 13, 2024

marc-farre commented Jan 13, 2024 • edited

luke- commented Jan 15, 2024

marc-farre commented Jan 16, 2024

luke- commented Jan 16, 2024

martin-rueegg commented Jan 17, 2024

marc-farre commented Jan 24, 2024

luke- commented Jan 24, 2024

marc-farre commented Jan 24, 2024

martin-rueegg commented Jan 24, 2024

martin-rueegg commented Jan 24, 2024

luke- commented Jan 24, 2024

marc-farre commented Jan 24, 2024

marc-farre commented Jan 25, 2024

luke- commented Jan 25, 2024

marc-farre commented Jan 25, 2024

luke- commented Jan 26, 2024

marc-farre commented Feb 1, 2024

luke- commented Feb 1, 2024

marc-farre commented Feb 1, 2024

marc-farre commented Feb 2, 2024

luke- commented Dec 15, 2023 •

edited

marc-farre commented Jan 13, 2024 •

edited