Add a "report output handler", so that we can start untangling the PDF, CSV

[mlutfy]

.. and email code, and allow extensions to implement their own methods (ex: Excel
Export).

Overview

Early attempt at untangling the Report code, so that it can be more easily pluggable.

Context: the "Export to Excel" extension (which has a fairly large user-base) requires a patch on core so that all reports export correctly:

https://lab.civicrm.org/extensions/civiexportexcel/-/blob/master/civiexportexcel-core.patch

As you can see, it really doesn't take much to make report export pluggable. Extensions can already add custom actions, but they can't influence the "outputMode".

I would appreciate feedback since:

• My understanding of object oriented programming is duct-tape-ish at best.
• I didn't feel like hooks were a right fit. This is more similar to a Payment Processor. At the same time, it might be introducing a new kind of pattern in CiviCRM, and maybe there is another pattern that already fits.

Ideally, I would like to start small, then implement this in the Export to Excel extension, iterate a bit (figure out if more methods should be added for other use-cases, then write good documentation.

Before

Impossible to export into anything else than CSV or PDF.

After

Extension authors can provide custom actions.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

I haven't tested this & it seems not quite review-ready (style, tests) but I agree with the approach & can't see any cause for concern in the code

I wrestled emotionally with this

  $className = 'CRM_Report_Output_' . ucfirst($this->_outputMode);
      if (class_exists($className)) {
        $this->_outputHandler = new $className($this);
      }

But I guess it's OK. The only alternative I can think of is some kind of array with pdf to class - which has i's own drawbacks I guess. It does feel a bi civi-4.x - perhaps @totten has thoughts

[demeritcowboy]

I noticed the same block of code too. There's often a preference for dependency-injection instead, where you ask for the class from the container. The advantage is that people can override the implementation of e.g. the csv output handler by specifying it via config, at the expense of some more abstraction.

So my two cents is at least separate that if block into its own factory class, e.g. OutputHandlerFactory, so it's easier to fiddle with later so it asks the container, possibly make it a singleton, etc...

e.g.

$this->_outputHandler = OutputHandlerFactory::getOutputHandler($this);

Others may have more opinions, or what might mesh better with civi.

[mlutfy]

Interesting - thanks for the feedback!

Just to double-check: would it make sense to create it under civicrm/Civi/Export/OutputHandler.php, then call it like this?

$this->_outputHandler = \Civi\Export\OutputHandler::create($this);

.. which would then wrap the code to create the new class, and later on we can make it configurable? The closest example I found was /Civi/Core/Themes.php, which has the "themes" hook?

and should I place the code under Civi? or is that more kept for services, and in this case, the difference betwen CRM and Civi is mostly a question of coding style?

[eileenmcnaughton]

I think above seems about right - I'd really like @totten to comment.

[seamuslee001]

Jenkins re test this please

[mlutfy]

Closing in favour of #17693