Patch #4665
open
Projects not calling EnabledModule callbacks in some cases
0%
Description
Implementation of Project.enabled_module_names= still calls modules.clear() in some cases (if nil is passed as parameter).
This has the risk of not invoking module callbacks (before_create, after_create, before_destroy, after_destroy) correctly.
Proposed change is to remove all traces of modules.clear(). Example:
def enabled_module_names=(module_names=[])
module_names = module_names.collect(&:to_s)
# remove disabled modules
enabled_modules.each {|mod| mod.destroy unless module_names.include?(mod.name)}
# add new modules
module_names.reject {|name| module_enabled?(name)}.each {|name| enabled_modules << EnabledModule.new(:name => name)}
end
Regards!
Updated by Felix Schäfer almost 14 years ago
Could you please give a way to reproduce any error this might cause?
Updated by Enrique Garcia almost 14 years ago
Ok.
This is the current implementation:
def enabled_module_names=(module_names)
if module_names && module_names.is_a?(Array)
module_names = module_names.collect(&:to_s)
# remove disabled modules
enabled_modules.each {|mod| mod.destroy unless module_names.include?(mod.name)}
# add new modules
module_names.reject {|name| module_enabled?(name)}.each {|name| enabled_modules << EnabledModule.new(:name => name)}
else
enabled_modules.clear # this doesn't invoke callbacks
end
end
I believe there is no way to reproduce this in "vanilla" Redmine - it never does project.enabled_modules = nil.
But it is a problem for plugin development.
The way the method is constructed, it seems to say "I accept either a list of modules, or nil, and I do different things depending on that".
The "nil" part isn't used on redmine, I believe. It is allways a list of modules. (I suspect this isn't included on the tests either). I believe the code there is wrong because enabled_modules.clear eliminates the list of modules without invoking the corresponding callbacks. (eg before_destroy) on the modules.
The proposed solution is more straightforward - the code says "give me a list of modules. if you give me nil, I'll make it an empty list". And callbacks are allways called. No "nil special case". It is also shorter.
I was "bitten" by this code myself when developing a plugin - was confused by the "if", tried to use it, and module callbacks stopped working, and I went bonkers for 3 days.
By the way, I submitted this question to Eric via email. The code on the OP is his - I copy-pasted it from his e-mail. He asked me to open a ticket and assign it to him, and I did so. I didn't notice that he did not action on it until today.
If you need any more clarifications, just let me know.
Updated by