Bug 21853

Summary: Internal software error when exporting basket group as PDF with Perl > 5.24.1
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: AcquisitionsAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P5 - low CC: dcook, fridolin.somers, jesse, m.de.rooy, mtompset, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22282
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 21853: Fix PDF export of basketgroups
Bug 21853: Follow-up to solve printpdf error
Bug 21853: Fix PDF export of basketgroups
Bug 21853: (follow-up) to solve printpdf error
Bug 21853: Fix PDF export of basketgroups
Bug 21853: (follow-up) to solve printpdf error

Description Julian Maurice 2018-11-16 17:13:16 UTC
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-search-_%22.%22_-for-optional-modules
Comment 1 Julian Maurice 2018-11-17 11:14:11 UTC
Created attachment 82422 [details] [review]
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
Comment 2 Julian Maurice 2018-11-17 11:16:44 UTC
Increasing severity. This needs to be pushed before 18.11 release
Comment 3 Mark Tompsett 2018-11-18 04:47:30 UTC
Comment on attachment 82422 [details] [review]
Bug 21853: Fix PDF export of basketgroups

Review of attachment 82422 [details] [review]:
-----------------------------------------------------------------

::: 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. :)
Comment 4 Mark Tompsett 2018-11-18 05:11:01 UTC
<h1>Software error:</h1>
<pre>Undefined subroutine &amp;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.
Comment 5 Mark Tompsett 2018-11-18 05:38:13 UTC
Created attachment 82425 [details] [review]
Bug 21853: Follow-up to solve printpdf error
Comment 6 Julian Maurice 2018-11-18 12:00:23 UTC
(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] [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 ?
Comment 7 Mark Tompsett 2018-11-19 03:53:53 UTC
> (In reply to M. Tompsett from comment #5)
> > Created attachment 82425 [details] [review] [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.
Comment 8 Mark Tompsett 2018-11-19 05:33:06 UTC
(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.
Comment 9 David Cook 2018-11-19 06:00:53 UTC
Oh this issue reminds me of something I've seen in the not so distant past...
Comment 10 Julian Maurice 2018-11-19 08:09:13 UTC
(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.
Comment 11 Marcel de Rooy 2018-11-19 08:38:36 UTC
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.
Comment 12 Julian Maurice 2018-11-19 08:54:17 UTC
(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.
Comment 13 Marcel de Rooy 2018-11-19 08:55:17 UTC
(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
Comment 14 Mark Tompsett 2018-11-19 13:05:00 UTC
(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.
Comment 15 Julian Maurice 2018-11-19 13:24:04 UTC
(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.
Comment 16 Mark Tompsett 2018-11-19 14:13:07 UTC
Created attachment 82498 [details] [review]
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>
Comment 17 Mark Tompsett 2018-11-19 14:13:10 UTC
Created attachment 82499 [details] [review]
Bug 21853: (follow-up) to solve printpdf error

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 18 Marcel de Rooy 2018-11-20 13:19:32 UTC
QA: Looking here now
Comment 19 Marcel de Rooy 2018-11-20 14:07:29 UTC
Created attachment 82545 [details] [review]
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>
Comment 20 Marcel de Rooy 2018-11-20 14:07:33 UTC
Created attachment 82546 [details] [review]
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..
Comment 21 Nick Clemens 2018-11-21 11:36:50 UTC
Awesome work all!

Pushed to master for 18.11
Comment 22 Jesse Maseto 2018-12-07 21:14:52 UTC
Backported to 18.05 for 18.05.07
Comment 23 Fridolin Somers 2018-12-19 07:11:38 UTC
Pushed to 17.11.x for 17.11.13