Fixed issue #17421: When upgrading from LS3 to LS4 / 5, plugin Manager can't be used if non-compatible plugins are found #2004
Conversation
…r can't be used if non-compatible plugins are found
|
Also see this issue: https://bugs.limesurvey.org/view.php?id=17516 |
There was a problem hiding this comment.
Also see this issue: https://bugs.limesurvey.org/view.php?id=17516
You mean to focus on this https://bugs.limesurvey.org/view.php?id=17516#c66011?
|
To highlight: If plugin is missing or is not compatible, it will be uninstalled |
Right, but this procedure might need to happen after db update too, not only when entering the plugin manager. |
…r can't be used if non-compatible plugins are found - Create PluginCleaner
…r can't be used if non-compatible plugins are found - Improve PluginCleaner
| try { | ||
| $config = $this->getExtensionConfig(); | ||
| return $config->isCompatible(); | ||
| } catch (\Exception $ex) { |
There was a problem hiding this comment.
Which exception are you catching here?
There was a problem hiding this comment.
Any exception from the getExtensionConfig or the isCompatible method.
Example: 'Missing configuration file for plugin %s, looked in location %s',
There was a problem hiding this comment.
Hm, do we want to silence that? Try-catch usually makes better sense higher up in the stacktrace, e.g. in a controller or such.
There was a problem hiding this comment.
By returning false we lose important information.
There was a problem hiding this comment.
There could be lot of malformations in config.xml files, missing config.xml files and even other situations not thought.
As so, any of those situations can be considered as a non compatibility.
By that is that we return false.
If not returning false, we should look up every usage of ->isCompatible() and add try/catch.
Still, not sure if that handling would be much different.
Maybe, to inform the error we could add an optional parameter with the exception caught? or add an attribute to store the nonCompatibleException
There was a problem hiding this comment.
Can't really merge this since all information in the exception is gone. An alternative could be to return [false, $ex->getMessage()]. Not sure that's much different than just not catch it, tho. :)
…r can't be used if non-compatible plugins are found - Fix error when plugin_type is invalid - Don't set flash messages on PluginCleaner - Add LSYii_Application::setFlashMessages() for convenience
…r can't be used if non-compatible plugins are found - Add missing file
| try { | ||
| $config = $this->getExtensionConfig(); | ||
| return $config->isCompatible(); | ||
| } catch (\Exception $ex) { |
There was a problem hiding this comment.
Can't really merge this since all information in the exception is gone. An alternative could be to return [false, $ex->getMessage()]. Not sure that's much different than just not catch it, tho. :)
|
Ahmed argues that this fix is not needed, it's not an issue. Can you still reproduce it, @gabrieljenik? Without this fix. |
|
Sorry, I'll close this for now. |
No description provided.