[Bug 21853] New: Internal software error when exporting basket group as PDF with Perl > 5.24.1
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Bug ID: 21853 Summary: Internal software error when exporting basket group as PDF with Perl > 5.24.1 Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Acquisitions Assignee: koha-bugs@lists.koha-community.org Reporter: julian.maurice@biblibre.com QA Contact: testopia@bugs.koha-community.org STR: 1. Go to Acquisitions module and create a vendor and a basketgroup 2. Close it and export it as PDF You should see a blank page saying "Internal Server Error" In the logs you should see Undefined subroutine &CGI::Compile::ROOT::home_koha_env_master_src_acqui_basketgroup_2epl::printpdf called Perl version is v5.24.1 and it is probably related to this change: http://perldoc.perl.org/perl5241delta.html#Core-modules-and-tools-no-longer-... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #1 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 82422 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82422&action=edit Bug 21853: Fix PDF export of basketgroups In recent versions of Perl, '.' is not included by default in @INC. This breaks PDF export of basketgroups. This patch moves acqui/pdfformat/*.pm files in Koha namespace so that they can be 'require'd without manipulating @INC Test plan: 1. Turn off Plack/Starman and test PDF export for every value of OrderPdfFormat system preference 2. Turn on Plack/Starman and test PDF export for every value of OrderPdfFormat system preference 3. Test on a dev install and a standard/package install -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |julian.maurice@biblibre.com |ity.org | Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|major |critical Keywords| |rel_18_11_candidate --- Comment #2 from Julian Maurice <julian.maurice@biblibre.com> --- Increasing severity. This needs to be pushed before 18.11 release -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #3 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 82422 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82422 Bug 21853: Fix PDF export of basketgroups Review of attachment 82422: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=21853&attachment=82422) ----------------------------------------------------------------- ::: acqui/pdfformat/layout2pages.pm @@ +1,1 @@
+package Koha::pdfformat::layout2pages;
I don't think you needed to remove the #!/usr/bin/perl, but merely add the Koha:: to the line below. -- Tested my idea, and yes this is overkill. ::: acqui/basketgroup.pl @@ +130,5 @@
+ if (grep {$_ eq $pdfformat} @valid_pdfformats) { + $pdfformat = "Koha::$pdfformat"; + my $pdfformat_filepath = File::Spec->catfile(split /::/, $pdfformat) . '.pm'; + require $pdfformat_filepath; + import $pdfformat;
Love this minor refactor. Much clearer and simpler. @@ +137,5 @@
+ print $input->header; + print $input->start_html; # FIXME Should do a nicer page + print "<h1>Invalid PDF Format set</h1>"; + print "Please go to the systempreferences and set a valid pdfformat"; + exit;
Over zealous white space fixing. :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |mtompset@hotmail.com --- Comment #4 from M. Tompsett <mtompset@hotmail.com> --- <h1>Software error:</h1> <pre>Undefined subroutine &main::printpdf called at /home/vagrant/kohaclone/acqui/basketgroup.pl line 212. </pre> <p> For help, please send mail to the webmaster (<a href="mailto:[no address given]">[no address given]</a>), giving this error message and the time and date of the error. </p> -- this happens on non-Plack. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #5 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 82425 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82425&action=edit Bug 21853: Follow-up to solve printpdf error -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #6 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to M. Tompsett from comment #3)
I don't think you needed to remove the #!/usr/bin/perl, but merely add the Koha:: to the line below. -- Tested my idea, and yes this is overkill. You're right. I didn't need to remove it, but a Perl module doesn't need a shebang line either ;)
Over zealous white space fixing. :) I plead guilty... :)
(In reply to M. Tompsett from comment #5)
Created attachment 82425 [details] [review] Bug 21853: Follow-up to solve printpdf error I don't need this patch even in CGI mode. All pdfformat::* modules automatically export their printpdf subroutine. Maybe some inconsistent Exporter behavior between different Perl versions ?
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #7 from M. Tompsett <mtompset@hotmail.com> ---
(In reply to M. Tompsett from comment #5)
Created attachment 82425 [details] [review] [review] Bug 21853: Follow-up to solve printpdf error I don't need this patch even in CGI mode. All pdfformat::* modules automatically export their printpdf subroutine. Maybe some inconsistent Exporter behavior between different Perl versions ?
Retested from scratch today (/var/log/koha/kohadev/intranet-error.log): [Mon Nov 19 03:40:11.053408 2018] [cgi:error] [pid 32166] [client 10.0.2.2:35866] AH01215: [Mon Nov 19 03:40:11 2018] basketgroup.pl: Useless use of greediness modifier '?' in regex; marked by <-- HERE in m/(\\S{20}? <-- HERE )(?=\\S)/ at /usr/share/perl5/PDF/Table.pm line 370.: /home/vagrant/kohaclone/acqui/basketgroup.pl, referer: http://localhost:8081/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=1&listclosed=1 And the CGI didn't generate anything. -- Hmmm... same error after my patch, so not relevant, I guess. Apply my patch: Got a PDF... with expected output. Painfully clear test plan: export LOCAL_ANSIBLE=1;vagrant destroy stretch;vagrant up stretch vagrant ssh stretch cd kohaclone git checkout master git pull git remote update -p git reset --hard origin/master git checkout -b bug_21853_retry origin/master git bz apply 21853 i -- just the first patch restart_all sudo service apache2 restart browse to localhost:8081 and run web installer log in to staff client Acquisitions. DOH! Need a budget. Add a budget. Acquisitions. DOH! Need a fund in the budget. Add a fund. Acquisitions. DOH! Need a vendor. Add vendor click Basket Groups on the left pane. New basket group save it Should be listed as open, click the 'close and export' type button. PDF should be generated and down loaded. show in file, just to make sure it really is a PDF. Yep. All good. sudo koha-list --plack OH! Kohadev is plack on. sudo koha-plack --disable kohadev restart_all sudo service apache2 restart Click the 'reopen' button. slower load time, this is good. click the 'close and export' type button. nothing?! Check the log file. Hmmm... git bz apply 21853 i just the second patch reopen and try again. PDF generated. PDF looks okay. I'm going to trying this all again for good measure. Just to make sure it wasn't some intermediate step that messed this up. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #8 from M. Tompsett <mtompset@hotmail.com> --- (In reply to M. Tompsett from comment #7) [SNIP]
I'm going to trying this all again for good measure. Just to make sure it wasn't some intermediate step that messed this up.
Okay, that's weird. It worked. Going to attempt other PDF systempreference settings... Okay... English 2 page in CGI is the failure case. Please confirm? With my patch it works. And what you said about them exporting is true, which makes it weird that my patch works. There must be some weird circular nesting or something. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #9 from David Cook <dcook@prosentient.com.au> --- Oh this issue reminds me of something I've seen in the not so distant past... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #10 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to M. Tompsett from comment #8)
Okay... English 2 page in CGI is the failure case. Please confirm? Sorry, I can't make it fail (haven't tried on kohadevbox though)
With my patch it works. And what you said about them exporting is true, which makes it weird that my patch works. There must be some weird circular nesting or something. Probably. Circular dependencies can do that sort of thing.
If the patch fixes it for you, I think that's a reason good enough to include it and not waste time trying to figure out what's happening. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Wouldn't it be easier to do: + $pdfformat = "acqui::$pdfformat"; and for each module: +package acqui::pdfformat::layout3pages; And move to the Koha namespace if they are refactored? Would be good candidates for more object oriented approach, i guess. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #12 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Marcel de Rooy from comment #11)
Wouldn't it be easier to do: + $pdfformat = "acqui::$pdfformat"; and for each module: +package acqui::pdfformat::layout3pages;
And move to the Koha namespace if they are refactored? Would be good candidates for more object oriented approach, i guess.
I think 'acqui' is not in @INC in a standard/package install. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Julian Maurice from comment #12)
(In reply to Marcel de Rooy from comment #11)
Wouldn't it be easier to do: + $pdfformat = "acqui::$pdfformat"; and for each module: +package acqui::pdfformat::layout3pages;
And move to the Koha namespace if they are refactored? Would be good candidates for more object oriented approach, i guess.
I think 'acqui' is not in @INC in a standard/package install.
Ah. Good point -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #14 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Marcel de Rooy from comment #11)
And move to the Koha namespace if they are refactored? Would be good candidates for more object oriented approach, i guess.
Valid point. Perhaps moving to C4 is the better first step? Koha should have beautiful refactored code. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #15 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to M. Tompsett from comment #14)
(In reply to Marcel de Rooy from comment #11)
And move to the Koha namespace if they are refactored? Would be good candidates for more object oriented approach, i guess.
Valid point. Perhaps moving to C4 is the better first step? Koha should have beautiful refactored code.
But all the code in Koha is beautiful ! (in its own special way :)) The coding guidelines says (PERL15):
Whenever it makes sense, code added to the Koha:: namespace should be object-oriented. However, code that is naturally procedural should not be shoehorned into the OO style. Modules in the Koha:: namespace should not reference the C4:: namespace, with the exception of C4::Context.
IMO it doesn't make sense for these modules to be in OO style and they don't use C4:: modules except C4::Context (a rule that is not even followed in master, just run `git grep C4 Koha/`). I don't see why these modules should be denied access to Koha namespace. I won't oppose moving them to C4 if you think it's better. There's just not enough justifications for it IMO. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #82422|0 |1 is obsolete| | Attachment #82425|0 |1 is obsolete| | --- Comment #16 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 82498 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82498&action=edit Bug 21853: Fix PDF export of basketgroups In recent versions of Perl, '.' is not included by default in @INC. This breaks PDF export of basketgroups. This patch moves acqui/pdfformat/*.pm files in Koha namespace so that they can be 'require'd without manipulating @INC Test plan: 1. Turn off Plack/Starman and test PDF export for every value of OrderPdfFormat system preference 2. Turn on Plack/Starman and test PDF export for every value of OrderPdfFormat system preference 3. Test on a dev install and a standard/package install Signed-off-by: Mark Tompsett <mtompset@hotmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 --- Comment #17 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 82499 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82499&action=edit Bug 21853: (follow-up) to solve printpdf error Signed-off-by: Mark Tompsett <mtompset@hotmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking here now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|BLOCKED |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #82498|0 |1 is obsolete| | --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 82545 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82545&action=edit Bug 21853: Fix PDF export of basketgroups In recent versions of Perl, '.' is not included by default in @INC. This breaks PDF export of basketgroups. This patch moves acqui/pdfformat/*.pm files in Koha namespace so that they can be 'require'd without manipulating @INC Test plan: 1. Turn off Plack/Starman and test PDF export for every value of OrderPdfFormat system preference 2. Turn on Plack/Starman and test PDF export for every value of OrderPdfFormat system preference 3. Test on a dev install and a standard/package install Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #82499|0 |1 is obsolete| | --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 82546 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=82546&action=edit Bug 21853: (follow-up) to solve printpdf error Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Theoretically this follow-up makes no difference. All modules only export printpdf and now we are just asking explicitly for printpdf. But if it resolves some exception on the rule.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com Status|Passed QA |Pushed to Master --- Comment #21 from Nick Clemens <nick@bywatersolutions.com> --- Awesome work all! Pushed to master for 18.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Jesse Maseto <jesse@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |jesse@bywatersolutions.com --- Comment #22 from Jesse Maseto <jesse@bywatersolutions.com> --- Backported to 18.05 for 18.05.07 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #23 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Pushed to 17.11.x for 17.11.13 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21853 Christophe Croullebois <christophe.croullebois@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=22282 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org