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(a)gmail.com> wrote:
On Wed, Aug 30, 2017 at 8:42 AM, Chris Travers <chris.travers(a)gmail.com>
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
* 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
* 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.
In the same paragraph, you continue
The Template MUST NOT assume HTTP, PSGI, or any other particular
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
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
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).
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
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?
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/
Can you be a bit more specific about what your proposal be changing to all
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.
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).
(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. 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. 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.
(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
-- Hosted accounting and ERP.
Robust and Flexible. No vendor lock-in.
Efficito: Hosted Accounting and ERP. Robust and Flexible. No vendor