Description
Marcel de Rooy
2014-11-07 15:35:37 UTC
Created attachment 33381 [details] [review] Bug 13223: Plugin housekeeping: do not redefine wrapper This report is connected to bug 10480 which will change the general mechanism of loading plugins, but can be tested independently and ahead of that proposed change. Several unimarc plugins use a wrapper sub. The code of this subroutine is not exactly the same for all plugins: in some cases the routine is extended for double character strings (dblspace and dblpipe). It would not hurt to use the extended code for all plugins. By moving the code to a module in the local dir, we prevent redefinition when two or more plugins are loading wrapper in a do-statement. NOTE: You will not see wrapper redefine errors in your log, since the plugins do not use the warnings pragma (yet). Since this patch touches seventeen unimarc plugins, a unimarc signoff is preferred :) Test plan: Use some plugins changed in this patch (if not in use already). Load the MARC editor. Click on some tag editor-buttons to check unchanged behavior. Good initiative. Shouldn't you put your module in C4 (C4::Utils) directory, rather than in plugin directory itself? (In reply to Frédéric Demians from comment #2) > Good initiative. Shouldn't you put your module in C4 (C4::Utils) directory, > rather than in plugin directory itself? Hi Frederic, I thought of that, but its use seems to be so connected to these plugins that I did not move it away from them. Do you prefer to have them there? > I thought of that, but its use seems to be so connected to these plugins
> that I did not move it away from them. Do you prefer to have them there?
Yes. I understand your reasoning. Except in misc/translator, I don't see any directory other than 'Koha' or 'C4' containing module files. We can imagine scenario in the future where Koha libraries could be searched differently than with PERL5LIB environment variable.
Another point: You module need a copyright, I suppose.
Created attachment 33484 [details] [review] Bug 13223: Plugin housekeeping: do not redefine wrapper This report is connected to bug 10480 which will change the general mechanism of loading plugins, but can be tested independently and ahead of that proposed change. Several unimarc plugins use a wrapper sub. The code of this subroutine is not exactly the same for all plugins: in some cases the routine is extended for double character strings (dblspace and dblpipe). It would not hurt to use the extended code for all plugins. By moving the code to a module, we prevent redefinition when two or more plugins are loading wrapper in a do-statement. NOTE: You will not see wrapper redefine errors in your log, since the plugins do not use the warnings pragma (yet). Since this patch touches seventeen unimarc plugins, a unimarc signoff is preferred :) Test plan: Use some plugins changed in this patch (if not in use already). Load the MARC editor. Click on some tag editor-buttons to check unchanged behavior. Created attachment 33489 [details] [review] Bug 13223: Plugin housekeeping: do not redefine wrapper This report is connected to bug 10480 which will change the general mechanism of loading plugins, but can be tested independently and ahead of that proposed change. Several unimarc plugins use a wrapper sub. The code of this subroutine is not exactly the same for all plugins: in some cases the routine is extended for double character strings (dblspace and dblpipe). It would not hurt to use the extended code for all plugins. By moving the code to a module, we prevent redefinition when two or more plugins are loading wrapper in a do-statement. NOTE: You will not see wrapper redefine errors in your log, since the plugins do not use the warnings pragma (yet). Since this patch touches seventeen unimarc plugins, a unimarc signoff is preferred :) Test plan: Use some plugins changed in this patch (if not in use already). Load the MARC editor. Click on some tag editor-buttons to check unchanged behavior. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Unimarc plugins work as usual. No regression. Simple code factorization. Thx Frederic. Could you upgrade the status? Created attachment 33493 [details] [review] Bug 13223: [QA Follow-up] Trivial change to one POD line The line referred to Plugin while it should be FrameworkPlugin. (I renamed the module in the process but forgot this line.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 33536 [details] [review] [PASSED QA] Bug 13223: Plugin housekeeping: do not redefine wrapper This report is connected to bug 10480 which will change the general mechanism of loading plugins, but can be tested independently and ahead of that proposed change. Several unimarc plugins use a wrapper sub. The code of this subroutine is not exactly the same for all plugins: in some cases the routine is extended for double character strings (dblspace and dblpipe). It would not hurt to use the extended code for all plugins. By moving the code to a module, we prevent redefinition when two or more plugins are loading wrapper in a do-statement. NOTE: You will not see wrapper redefine errors in your log, since the plugins do not use the warnings pragma (yet). Since this patch touches seventeen unimarc plugins, a unimarc signoff is preferred :) Test plan: Use some plugins changed in this patch (if not in use already). Load the MARC editor. Click on some tag editor-buttons to check unchanged behavior. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Unimarc plugins work as usual. No regression. Simple code factorization. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 33537 [details] [review] [PASSED QA] Bug 13223: [QA Follow-up] Trivial change to one POD line The line referred to Plugin while it should be FrameworkPlugin. (I renamed the module in the process but forgot this line.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I know that the new module is quite simple, but please add some unit tests :) (In reply to Katrin Fischer from comment #11) > I know that the new module is quite simple, but please add some unit tests :) Hi Katrin: This is really overkill imo.. Created attachment 33542 [details] [review] Bug 13223: Follow-up - Adding some unit tests My try at some unit tests... hope they are ok. Created attachment 33546 [details] [review] Bug 13223: Follow-up - Adding some unit tests Signed-off-by: Frederic Demians <f.demians@tamil.fr> Created attachment 33547 [details] [review] Bug 13223: [QA Follow-up] Adding some unit tests for wrapper Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Trivially amended. Thanks, Katrin. Looks like a collision :) We must be careful that Koha::Util does not get messy. IMO we should not use it at all, that force developers to use something else. Jonathan - where would you put it? (In reply to Jonathan Druart from comment #18) > We must be careful that Koha::Util does not get messy. > IMO we should not use it at all, that force developers to use something else. I agree on the first line. But the four line wrapper thing is a typical example of a util. (Note that I initially left it in cataloguing/value_builder, but was asked to move it to a util folder.) (In reply to Katrin Fischer from comment #19) > Jonathan - where would you put it? I don't know :) Koha::Cataloguing::Plugin? (In reply to M. de Rooy from comment #20) > I agree on the first line. But the four line wrapper thing is a typical > example of a util. (Note that I initially left it in > cataloguing/value_builder, but was asked to move it to a util folder.) At some points, all can be considered as a "util" :) It was just a quick note, I did not want to block this patch. (In reply to Jonathan Druart from comment #21) > (In reply to Katrin Fischer from comment #19) > > Jonathan - where would you put it? > > I don't know :) > Koha::Cataloguing::Plugin? No please, bug 10480 will add a real object for plugins. Work in progress.. > At some points, all can be considered as a "util" :) > It was just a quick note, I did not want to block this patch. Great. Bit worried about adding 'plugin' confusion, as we also have the younger plugin system and TT plugins... (In reply to Katrin Fischer from comment #23) > Bit worried about adding 'plugin' confusion, as we also have the younger > plugin system and TT plugins... Thats why I renamed to FrameworkPlugin. Patches pushed to master. Great job Marcel! |