Bug 31373

Created attachment 139210 [details] [review]
Bug 31373: Add INCLUDE_PATH to notice validation

This is a follow-up to bug 31211. It adds the same INCLUDE_PATH
variables as C4::Templates adds during processing to prevent false
failures.

Test plan
1) Add an INCLUDE to any valid notice, for example:
   [% INCLUDE 'patron-title.inc' patron => patron %]
2) Save and continue
3) Confirm you are faced with a validation error complaining that
   the include file cannot be found
4) Apply this patch
5) Confirm that the reported error has dissapeared

Created attachment 139211 [details] [review]
Bug 31373: Add INCLUDE_PATH to notice validation

This is a follow-up to bug 31211. It adds the same INCLUDE_PATH
variables as C4::Templates adds during processing to prevent false
failures.

Test plan
1) Add an INCLUDE to any valid notice, for example:
   [% INCLUDE 'patron-title.inc' patron => patron %]
2) Save and continue
3) Confirm you are faced with a validation error complaining that
   the include file cannot be found
4) Apply this patch
5) Confirm that the reported error has dissapeared

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Why do we process html_helpers?

Why are we duplicating C4::Letters::_process_tt?

(In reply to Jonathan Druart from comment #3)
> Why do we process html_helpers?

Ha, this crept in from another bug I was working... I found that including biblio-title.inc requires a pre-process of html_helpers and it felt like a good one to add to the default process rather than requiring people to know they need to have it processed at the top in the template itself.  It should probably be in it's own bug.
 
> Why are we duplicating C4::Letters::_process_tt?

So, I did wonder this myself.. I think it's because we don't have any parameters to actually pass in at this point.. so we just want a compile check.. must admit I didn't dig into the depths of the original bug.

(In reply to Martin Renvoize from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Why are we duplicating C4::Letters::_process_tt?
> 
> So, I did wonder this myself.. I think it's because we don't have any
> parameters to actually pass in at this point.. so we just want a compile
> check.. must admit I didn't dig into the depths of the original bug.

IMO we should stop as soon as possible and fix the situation, instead of adding more code to the controller.

(In reply to Martin Renvoize from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Why do we process html_helpers?
> 
> Ha, this crept in from another bug I was working... I found that including
> biblio-title.inc requires a pre-process of html_helpers and it felt like a
> good one to add to the default process rather than requiring people to know
> they need to have it processed at the top in the template itself.  It should
> probably be in it's own bug.

Preferable to include it explicitly when you need it instead of adding black magic. And it won't help plugins to be compatible between different versions if sometimes it needs to be included, sometimes not.

(In reply to Jonathan Druart from comment #6)
> (In reply to Martin Renvoize from comment #4)
> > (In reply to Jonathan Druart from comment #3)
> > > Why do we process html_helpers?
> > 
> > Ha, this crept in from another bug I was working... I found that including
> > biblio-title.inc requires a pre-process of html_helpers and it felt like a
> > good one to add to the default process rather than requiring people to know
> > they need to have it processed at the top in the template itself.  It should
> > probably be in it's own bug.
> 
> Preferable to include it explicitly when you need it instead of adding black
> magic. And it won't help plugins to be compatible between different versions
> if sometimes it needs to be included, sometimes not.

Fair enough.. it should be very clearly documented somewhere which includes require that helper then so notice authors aren't surprised by them not working.

I'd also remove the KohaDates and Remove_MARC_punctuation additions too as they suffer the same issues.

If we remove KohaDates now, that might be a little late. Same for the other TT filter also probably not as widely used. 

What about notices not including the USE already?

Created attachment 139406 [details] [review]
Bug 31373: DRY - Use try/catch around _process_tt

This patch reduces the repetition in the tools/letter.pl controller
replaceing it with a try/catch block wrapping a call to
C4::Letters::_process_tt instead.

The point I was trying to make with the including html_helpers was that I don't see how it's different from automagically including the other two we include.. to me, this is a well hidden undocumented template thing so made sense to 'just make it easy'.. Anywho.. it should certainly be in it's own bug.

Moving on.. please review my follow-up/alternative (Let's golf it then squash).

I don't like the regex mangling of the croak output but was failing to work out how else to clean up the resulting output to screen... we could remove the croak and get more meaningful in our return from the internal C4::Letter function.. but that also felt a bit funky.

Created attachment 139677 [details] [review]
Bug 31373: DRY - Use try/catch around _process_tt

This patch reduces the repetition in the tools/letter.pl controller
replaceing it with a try/catch block wrapping a call to
C4::Letters::_process_tt instead.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 140456 [details] [review]
Bug 31373: Add INCLUDE_PATH to notice validation

This is a follow-up to bug 31211. It adds the same INCLUDE_PATH
variables as C4::Templates adds during processing to prevent false
failures.

Test plan
1) Add an INCLUDE to any valid notice, for example:
   [% INCLUDE 'patron-title.inc' patron => patron %]
2) Save and continue
3) Confirm you are faced with a validation error complaining that
   the include file cannot be found
4) Apply this patch
5) Confirm that the reported error has dissapeared

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 140457 [details] [review]
Bug 31373: DRY - Use try/catch around _process_tt

This patch reduces the repetition in the tools/letter.pl controller
replaceing it with a try/catch block wrapping a call to
C4::Letters::_process_tt instead.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Pushed to master for 22.11.

Nice work everyone, thanks!