Summary: | Get rid of redefined subroutine warnings from dateaccessioned.pl | ||
---|---|---|---|
Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
Component: | Architecture, internals, and plumbing | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | minor | ||
Priority: | P5 - low | CC: | jonathan.druart, tomascohen, veron |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Trivial patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Attachments: |
Bug 128840: Get rid of redefined subroutine warnings in dateaccessioned.pl
Bug 12884: Get rid of redefined subroutine warnings in dateaccessioned.pl [Signed-off] Bug 12884: Get rid of redefined subroutine warnings in dateaccessioned.pl [ALT] Bug 12884: Get rid of redefined subroutine warnings in dateaccessioned.pl Bug 12884: Get rid of redefined subroutine warnings in dateaccessioned.pl |
Description
Marcel de Rooy
2014-09-08 11:35:57 UTC
Created attachment 31452 [details] [review] Bug 128840: Get rid of redefined subroutine warnings in dateaccessioned.pl 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.. ;) Test plan: Edit an item. Check log. Apply patch. Edit an item. Check log. Created attachment 31453 [details] [review] Bug 12884: Get rid of redefined subroutine warnings in dateaccessioned.pl 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.. ;) Test plan: Edit an item. Check log. Apply patch. Edit an item. Check log. I don't see any errors in my log related to dateaccessioned.pl. What steps should I take to reproduce this problem? Created attachment 31578 [details] [review] [Signed-off] Bug 12884: Get rid of redefined subroutine warnings in dateaccessioned.pl 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.. ;) Test plan: Edit an item. Check log. Apply patch. Edit an item. Check log. Without patch I got warnings in intranet-error.log. With patch no more warnings occured. Signed-off-by: Marc Véron <veron@veron.ch> (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. Created attachment 31632 [details] [review] [ALT] 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. Marcel, could you have a look at my alternative patch please? 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> This works fine too, Jonathan. Taking the liberty to promote this directly to Passed QA now. We spent enough time on this trivial one. (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? (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. Patch pushed to master. Thanks Jonathan! |