Bug 13223 - Plugin housekeeping: do not redefine wrapper
Summary: Plugin housekeeping: do not redefine wrapper
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-07 15:35 UTC by Marcel de Rooy
Modified: 2015-06-04 23:33 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13223: Plugin housekeeping: do not redefine wrapper (14.18 KB, patch)
2014-11-07 16:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13223: Plugin housekeeping: do not redefine wrapper (15.40 KB, patch)
2014-11-12 13:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13223: Plugin housekeeping: do not redefine wrapper (15.49 KB, patch)
2014-11-12 14:57 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13223: [QA Follow-up] Trivial change to one POD line (951 bytes, patch)
2014-11-12 16:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PASSED QA] Bug 13223: Plugin housekeeping: do not redefine wrapper (15.56 KB, patch)
2014-11-13 20:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13223: [QA Follow-up] Trivial change to one POD line (974 bytes, patch)
2014-11-13 20:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13223: Follow-up - Adding some unit tests (1.02 KB, patch)
2014-11-13 22:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13223: Follow-up - Adding some unit tests (1.07 KB, patch)
2014-11-14 07:29 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13223: [QA Follow-up] Adding some unit tests for wrapper (1.11 KB, patch)
2014-11-14 07:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2014-11-07 15:35:37 UTC

    
Comment 1 Marcel de Rooy 2014-11-07 16:10:17 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2014-11-10 13:10:32 UTC
Good initiative. Shouldn't you put your module in C4 (C4::Utils) directory, rather than in plugin directory itself?
Comment 3 Marcel de Rooy 2014-11-10 13:38:37 UTC
(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?
Comment 4 Frédéric Demians 2014-11-10 15:00:16 UTC
> 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.
Comment 5 Marcel de Rooy 2014-11-12 13:24:37 UTC Comment hidden (obsolete)
Comment 6 Frédéric Demians 2014-11-12 14:57:17 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2014-11-12 15:11:48 UTC
Thx Frederic. Could you upgrade the status?
Comment 8 Marcel de Rooy 2014-11-12 16:04:41 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2014-11-13 20:58:39 UTC
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>
Comment 10 Katrin Fischer 2014-11-13 20:58:59 UTC
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>
Comment 11 Katrin Fischer 2014-11-13 20:59:40 UTC
I know that the new module is quite simple, but please add some unit tests :)
Comment 12 Marcel de Rooy 2014-11-13 21:32:03 UTC
(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..
Comment 13 Katrin Fischer 2014-11-13 22:06:54 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2014-11-13 22:07:30 UTC
My try at some unit tests... hope they are ok.
Comment 15 Frédéric Demians 2014-11-14 07:29:18 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2014-11-14 07:30:14 UTC
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.
Comment 17 Marcel de Rooy 2014-11-14 07:30:56 UTC
Looks like a collision :)
Comment 18 Jonathan Druart 2014-11-14 08:25:45 UTC
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.
Comment 19 Katrin Fischer 2014-11-14 08:28:51 UTC
Jonathan - where would you put it?
Comment 20 Marcel de Rooy 2014-11-14 08:30:24 UTC
(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.)
Comment 21 Jonathan Druart 2014-11-14 08:45:46 UTC
(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.
Comment 22 Marcel de Rooy 2014-11-14 08:48:25 UTC
(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.
Comment 23 Katrin Fischer 2014-11-14 09:11:36 UTC
Bit worried about adding 'plugin' confusion, as we also have the younger plugin system and TT plugins...
Comment 24 Marcel de Rooy 2014-11-14 09:15:05 UTC
(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.
Comment 25 Tomás Cohen Arazi 2014-11-19 16:32:33 UTC
Patches pushed to master.

Great job Marcel!