Bug 21853

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

Increasing severity. This needs to be pushed before 18.11 release

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. :)

<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.

Created attachment 82425 [details] [review]
Bug 21853: Follow-up to solve printpdf error

(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 ?

> (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.

(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.

Oh this issue reminds me of something I've seen in the not so distant past...

(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.

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.

(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.

(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

(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.

(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.

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>

Created attachment 82499 [details] [review]
Bug 21853: (follow-up) to solve printpdf error

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

QA: Looking here now

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>

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..

Awesome work all!

Pushed to master for 18.11

Backported to 18.05 for 18.05.07

Pushed to 17.11.x for 17.11.13