On Thu, Aug 31, 2017 at 7:40 AM, Chris Travers <chris.travers@gmail.com> wrote:
Hi.  First, yes it is good to focus on definitions and consistent vocabulary.  Explanation to follow, along with and will modify RFC to match.

On Wed, Aug 30, 2017 at 8:57 PM, Erik Huelsmann <ehuels@gmail.com> wrote:

On Wed, Aug 30, 2017 at 8:42 AM, Chris Travers <chris.travers@gmail.com> wrote:
I have set up an issue with a more concrete proposal for better separation of concerns in the templates.



Thanks for the write-up. To be 100% sure we understand each other, let me explain my vocabulary before proceeding:

* Template: the actual file holding the structure of the output and the instructions for the template processor (i.e. these would be our files in UI/*.html)
* Template processor: The software transforming the template to the intended output (this would be Template Toolkit)
* LedgerSMB Template Format (or 'format' in short): the plugins into the template processor wrapper which implement format specific operations like format encoding
* LedgerSMB Template Engine (LedgerSMB::Template): the module which wraps the template processor; the glue layer between the LedgerSMB application and the Template Toolkit

Fair enough.  Let's go with these terms. 
 
In the referenced issue, you write

The template should be responsible only to render the content and provide a file mime type back with it. This means that the output of template->render() should ...

Assuming that "template" there corresponds to my definition of "LedgerSMB Template Engine" (concluded from the fragment of the second sentence), I really wonder what value the template engine is providing: our template processor has a render method too; the *only* thing it lacks is the content type return value. The content type can be easily added based on the extension of the processed template in a generic handler elsewhere though.

Right. 

In the same paragraph, you continue

The Template MUST NOT assume HTTP, PSGI, or any other particular input/output format.

Could you have a look and https://github.com/ledgersmb/LedgerSMB/blob/master/lib/LedgerSMB/Template.pm#L540 ? I think that that achieves what you want.

Yes, though more specifically, I think if we *do not use* PSGI or output options we achieve something much better.  What I want to achieve is a clearer separation of responsibility and I think this has cascading benefits through the rest of the application as well as making the logic more general.

Ok. You're proposing the return value being a pair: the expanded template and the mime-type. The current implementation is a list of headers (usually just the mime-type), the expanded template and a success/fail indicator (a triplet).

If we want to go with just the pair, how are we going to report the success/fail indicator? (In other words, how do we report error conditions?) I'll move the handling of the cache control headers currently in "render_to_psgi()" to LedgerSMB::PSGI. That's a positive outcome of this discussion for sure.

Elsewhere in the issue you refer to "the main wrapper script". Could you be a bit more specific about that? Is that a wrapper to be created around LedgerSMB::Template, is that LedgerSMB::Template itself, is that one of the modules in LedgerSMB::Scripts or is that one of LedgerSMB::PSGI or tools/starman.psgi ?

Yes, the LedgerSMB::PSGI or whatever replaces it in the future.  In other words the wrapper should be the one to decide what headers to send (and should have all PSGI-related functionality). 

Well, Plack already decides about a lot of headers for us: Transfer-Encoding (in case of 'chunked') and 'Content-Length' are never set by the template nor by the the report "render_to_psgi" methods. They're added by Plack.
 
I believe this would allow us to isolate PSGI and CGI-related code in one or two places only (request parsing and the request wrappers) for new code and break them entirely out of the template's responsibility.

We currently have what I call "PSGI or CGI related code" in LedgerSMB::PSGI (a lot), in old/lib/LedgerSMB/old_code.pm (a wrapper completely dedicated to the cgi/psgi difference and isolating running of old code in a forked interpreter) and a little bit in lib/LedgerSMB.pm and lib/LedgerSMB/Form.pm.

But looking at it, the PSGI.pm doesn't decide what headers to send does it?  It gets this from the workflow script which gets it from the template engine, is that correct? 

Only for cache control (which I'll move to LedgerSMB::PSGI due to this review/discussion -- a good improvement); that leaves the content type and the content-disposition headers. the latter are derived from the content-type; so, the disposition can be moved to LedgerSMB::PSGI too. That just leaves the mime type header (content-type). Which is the level we want to end up with.
 
Other than that, the CGI assumption is omni-present in old/bin/*.pl, as you know. I don't think you're talking about replacing that (other than replacing the entire code in that directory, which is why it's in the old/ directory).

Can you be a bit more specific about what your proposal be changing to all that?

I would like to work on getting rid of the render_to_psgi and the output functions from the template engine, and return the file type with the contents.  I think to make this work, we would need to provide a compatibility class for old code.

Ok. What is the problem with the render_to_psgi() functions when you throw away the success/fail indicator and convert that to a 'die()' when the value isn't equal to HTTP_OK ? I mean, that amounts to the same thing as the pair you're proposing? The body is one of two things: an array of strings or a filehandle. That should be easy to convert to a string (as you want it?).
 
Here's a case in point:  Suppose I want a cron job which emails a spreadsheet of overdue invoices (ageing detail report).  I think this should be a fairly easy thing to do even with the global state requirements, but on 1.4 it goes something like "fork our reporting engine" and the problem still is not fully fixed in master (worse, it is not *obvious* that the problem is not fully fixed in master because of assumptions in the templating engine).

I'm afraid I'm missing the point. In my mind you'd need to instantiate a LedgerSMB::Report::Aging, call its "render_to_psgi()" method and take the body out of the returned triple. Then you do with that body whatever you want.

I'm not sure I see what the problem is that isn't completely fixed in master.
 
(Note that I'm avoiding to say that we have resolved the problem that none of our modules behave as "good libraries" and make lots of assumptions about the context in which they're being called, like accessing and/or assuming global state having been set up.)

Agreed.  One of the point of trying to formalise things that can be broken off is to make good libraries and a smaller footprint.  But this is about two things.  The first is general interface and having an interface which avoids assuming that every component is being accessed from the whole framework.

Yes. We completely agree there.


WAIT! While going over the response above, I'm suddenly realising your use-case is with the aging report!

Then it's my time now to say that you're confusing one problem for another. Our LedgerSMB::Template is conflating two completely unrelated (although both "template" related) problems: LedgerSMB::Template generates our UI *and* LedgerSMB::Template generates our templatized documents.
While doing the latter, it even mistakenly sends them as "UI responses" (HTTP responses).

I've long wanted to separate the "UI templates processor" from the "document templates processor". Having that separation resolves 99% of the cases where people may want to run reports from outside the context of a web request.

I'm not convinced that separating "render_to_psgi()" out from the "UI templates processor" actually brings us anything: Dancer's "template()" function is very tightly nit to the response being sent too.

  I think these two are closely connected and global state is a part of the second.
 
The case that again immediately occurred to me here was the fact that LedgerSMB::Report->render sends the result in CGI format, despite the fact that this is a module introduced in 1.4.

We didn't "do" PSGI-native-responses until 1.6, so, both 1.4 and 1.5 return CGI responses which are being converted to PSGI responses on the request-dispatch boundary.
 
  And even in master, you have a choice of PSGI or CGI only (because for historical reasons we output to cgi by default).  Yes, we can fix it in the reporting engine by adding a line of code but I guess I see this as a single responsibility problem because the templating engine takes on the responsibility of output which I think is a problem.

The problem is that we have a single templating engine to generate our UI output as well as our "documents" (invoices, reports,...) output. (And because they were conflated, we stored the report templates in the UI/ tree, instead of in the templates/ tree (!), with the huge downside that they're not translatable or per-locale modifiable.)
 
(But if it doesn't take on the responsibility to output then I don't think it should take on PSGI responsibility either.)

There is a related question here which is where should the HTTP status messages come from but I think that's an additional discussion. I think they should only come from the workflow scripts or from PSGI.pm but open to other ideas.

Now we're getting back to the beginning of the discussion: if the UI templating engine doesn't take care of generating the actual output, then what role *does* it have? I mean, expanding the template is a function already performed by Template Toolkit...



--
Bye,

Erik.

http://efficito.com -- Hosted accounting and ERP.
Robust and Flexible. No vendor lock-in.