Better handling of legacy configuration file options for disabled modules #5629
Comments
|
Personally I believe that the My suggestion would be making a list (which is already in the community site) of variables that are less used ordered by recommendation while creating the settings for admins in the admin panel for more important variables. This would reduce the need for the use of a |
|
If think it would be great if this issue is solved. |
|
@funkycram Would be great if you can take a look into it. |
|
@luke- For the moment, I didn't succeed to find a solution for it (I'll try later if I find more time). FYI, this issue crashed my updated to HumHub 1.16 because I had this in my 'search' => [
'searchItemLimit' => 10000,
],I'm afraid many of us are experiencing this bug. Error log: |
|
Here's an idea that could probably help mitigate such issues, similar to the 3rd-party Module Example<?php
/**
* This file provides to overwrite the default HumHub / Yii configuration by your local common (Console and Web) environments
* @see http://www.yiiframework.com/doc-2.0/guide-concept-configurations.html
* @see http://docs.humhub.org/admin-installation-configuration.html
* @see http://docs.humhub.org/dev-environment.html
*/
// Default configuration values
$config = [
'common_variable' => 'Default value for common variable',
];
// Check for installed modules
$moduleManager = Yii::$app->getModuleManager();
$installedModules = $moduleManager->getModules();
// Merge installed modules' configurations
foreach ($installedModules as $moduleId => $moduleClass) {
$variablesFile = Yii::getAlias('@' . $moduleId . '/variables.php');
if (file_exists($variablesFile)) {
$moduleConfig = require($variablesFile);
// Merge the module-specific configurations with the default configuration
$config = array_merge($config, $moduleConfig);
}
}
return $config;
|
|
@marc-farre If you have time, maybe you can test my PR. This should prevent errors caused by old config variables. |
|
@luke- Thanks very much for this PR! I've tested to add this in 'modules' => [
'space' => [
'missingProperty' => 'some value',
],
],And I have: But I think your PR should help me find a solution. I'll try to work on it as soon as possible (if I manage to work it out!). |
|
@marc-farre My PR should only protect/warn if there are outdated configuration entries. (e.g. Search/Directory) There is still an error for incorrect configuration entries. That is intentional. |
|
@luke- I see. New test:
'modules' => [
'test-module' => [
'icon' => 'gear',
],
],
I still have the error: |
* Added handling for legacy configuration file options * Code Style fixes * Fix #5629: Better handling of legacy configuration file options * Removed unused imports * Fixed typo
|
@luke- FYI, I have merged the PR - It only covers core related scenario with changed modules/settings (e.g. removal of modules such as search) and prevents a fatal error and displays a warning in the SelfTest. For other scenarios we need different checks:
However, it would be important that we do not suppress such errors - instead we should show a more meaningful error. |
marc-farre
changed the title
marc-farre
changed the title
|
@luke- thanks! 1/ Custom configurations of not existent modulesI've started a PR here: https://github.com/humhub/humhub/pull/6938/files I wanted to throw an So with the PR, a flash error is displayed, and the same message is added as an error in the log:
If you agree with the PR, I'll fix the test issues. 2/ Invalid properties of modules and componentsWe already have an exception explaining the error, so I think it's enough for now (even if we could later explain where to find the configuration containing this unknown property): https://github.com/yiisoft/yii2/blob/master/framework/base/Component.php#L210 |
|
@marc-farre Thanks looks good. Is it ready to merge or is something to do or test? |
Yes. Now checks have passed. |
#6938) * Fix #5629: Better handling of legacy configuration file options for disabled non-core modules * Fix #5629: Better handling of legacy configuration file options for disabled non-core modules * Fix #5629: Better handling of legacy configuration file options for disabled non-core modules * Fix #5629: Better handling of legacy configuration file options for disabled non-core modules
Is your feature request related to a problem? Please describe.
Add a module setting in the
protected/config/common.phpfile.Uninstall the module or remove or rename the attribute in the
module.phpthat is used in common.php.Humhub crashes (see log bellow).
Describe the solution you'd like
Humhub should ignore modules names or attributes names that doesn't exists and add an error in the log to prevent the admin to update the common.php configuration file.
Describe alternatives you've considered
Add a notice in https://docs.humhub.org/docs/admin/troubleshooting
Error log:
The text was updated successfully, but these errors were encountered: