Bug 12884 - Get rid of redefined subroutine warnings from dateaccessioned.pl
Summary: Get rid of redefined subroutine warnings from dateaccessioned.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-08 11:35 UTC by Marcel de Rooy
Modified: 2015-06-04 23:33 UTC (History)
3 users (show)

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


Attachments
Bug 128840: Get rid of redefined subroutine warnings in dateaccessioned.pl (1.25 KB, patch)
2014-09-08 11:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12884: Get rid of redefined subroutine warnings in dateaccessioned.pl (1.25 KB, patch)
2014-09-08 11:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[Signed-off] Bug 12884: Get rid of redefined subroutine warnings in dateaccessioned.pl (1.44 KB, patch)
2014-09-13 14:43 UTC, Marc Véron
Details | Diff | Splinter Review
[ALT] Bug 12884: Get rid of redefined subroutine warnings in dateaccessioned.pl (1.06 KB, patch)
2014-09-16 13:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12884: Get rid of redefined subroutine warnings in dateaccessioned.pl (1.16 KB, patch)
2014-09-16 14:27 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-09-08 11:35:57 UTC
Most value_builder scripts have use warnings commented since they redefine subroutines like plugin_javascript etc. and they would create a lot of loglines.
The dateaccessioned script in this folder does not have it commented; so it creates loglines.

While we wait for the real solution for those redefines, I am at least consistent when commenting the use warnings here.. ;)
Comment 1 Marcel de Rooy 2014-09-08 11:37:37 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2014-09-08 11:59:17 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2014-09-10 15:36:35 UTC
I don't see any errors in my log related to dateaccessioned.pl. What steps should I take to reproduce this problem?
Comment 4 Marc Véron 2014-09-13 14:43:36 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2014-09-15 15:59:27 UTC
(In reply to Owen Leonard from comment #3)
> I don't see any errors in my log related to dateaccessioned.pl. What steps
> should I take to reproduce this problem?

This has to do with the order in which the plugins are defined/redefined.
Create an item with a subfield d (dateaccessioned) and p (barcode).
Verify that both are connected to a plugin (should be by default).
If d and p are the only ones loaded and d goes before p, you will have no warnings.

The order of plugin definition depends on additem line 771:
  foreach my $subfield ( $field->subfields() )
So it depends on MARC::Field; the internal _subfields in MARC::Field seems not to be sorted; new subfields are added at the end.

An easy way to force this problem, is add a sort at line 771:
  foreach my $subfield ( sort { $a->[0] lt $b->[0] } $field->subfields() )
In my simple test case p now goes before d and you should have warnings.

Note that I had the errors by default in 3.14 and not in master. But it just depends on circumstances, data etc. If you add a plugin before d, you could probably force it too.
Comment 6 Jonathan Druart 2014-09-16 13:34:04 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-09-16 13:34:46 UTC
Marcel, could you have a look at my alternative patch please?
Comment 8 Marcel de Rooy 2014-09-16 14:27:15 UTC
Created attachment 31636 [details] [review]
Bug 12884: Get rid of redefined subroutine warnings in dateaccessioned.pl

Instead of removing all warnings, we should remove warnings about the
redefined subroutines.

Test plan:
- link the dateaccessioned plugin with one of your biblio field
- edit an item
- verify they are no warnings in the Koha log file.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2014-09-16 14:28:47 UTC
This works fine too, Jonathan.
Taking the liberty to promote this directly to Passed QA now. We spent enough time on this trivial one.
Comment 10 Tomás Cohen Arazi 2014-10-18 21:12:11 UTC
(In reply to M. de Rooy from comment #9)
> This works fine too, Jonathan.
> Taking the liberty to promote this directly to Passed QA now. We spent
> enough time on this trivial one.

Aren't we just hiding a problem?
Comment 11 Marcel de Rooy 2014-10-19 12:07:21 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> (In reply to M. de Rooy from comment #9)
> > This works fine too, Jonathan.
> > Taking the liberty to promote this directly to Passed QA now. We spent
> > enough time on this trivial one.
> 
> Aren't we just hiding a problem?

Problem is a big word. But yes, we are hiding the warnings just as all other value builder scripts do. It should be/have been designed differently.

BTW I tried something on bug 10480 with a global variable. But I was not satisfied with it.
What perhaps made it more complex, was the gradual approach: keep the old way alive, while introducing the new way.

Will still think about something else, but I do not object to this patch.
Comment 12 Tomás Cohen Arazi 2014-10-22 17:32:27 UTC
Patch pushed to master.

Thanks Jonathan!