Re: [VuFind-Tech] ServiceLocatorization

[David M. <ma...@ha...>]

At Mon, 17 Sep 2012 16:03:49 +0000,
Demian Katz wrote:
>
> I'm addressing this one point in a separate email since it doesn't directly bear on the Solr refactoring.  I'll respond to the remainder of your PR-related emails after I have had time to look at the PRs (my main task for the remainder of the day).
>
> > @Demian: I think this is case where ServiceLocatorization has gone to far. For
> > the QueryBuilder loading the search specs is a one-shot call:
> > load(filename) returns array(). AFAIK the SearchSpecLoader is a specialized
> > generic YAML loader w/ support for caching. There's nothing the QueryBuilder
> > needs to setup -- A call to a static method is totally sufficient and a
> > simpler solution than to pass a service locator all way down.
> >
> > I have the same feeling for the logger: AFAIK every class that needs logging
> > performs a one-shot call: log(PRIORITY, MESSAGE) - done. What the logger does
> > with this is totally irrelevant for the class using a logger. Internally the
> > Logger might use different logging instances or whatever -- doesn't matter for
> > the user of the class.
>
> In both of these situations, the service manager is not being used simply to make it more convenient to load a service -- it is also being used to ensure that those services are able to find their own dependencies.  A static call would be simpler, but that would require either a) manual dependency injection or b) that resources loaded statically also load their dependencies statically, which reduces the flexibility and extensibility of VuFind in general.
>
> To be more specific:
>
> \VuFind\Config\SearchSpecsReader has a soft dependency on \VuFind\Cache\Manager.  We need to load it through the service manager or it doesn't know how to find the object it needs to cache data.
>
> \VuFind\Log\Logger has a soft dependency on \VuFind\Mailer, which it uses when email logging is enabled.  (Actually, this is more of a hard dependency at the moment, in the sense that omitting the mailer service from the service locator will cause a fatal error...  but this could be worked around if there were a compelling need; given that \VuFind\Log\Logger is a VuFind-specific tool, I don't see a compelling need).
>
> Or, to put it another way, the Service Locator provides a centralized alternative to having static instances scattered all over the place.  The problem is figuring out where to draw the line.  At this point, I feel that it's better to use the Service Locator, even if it's slightly inconvenient, because it enhances extensibility (you can override classes from a local module) and provides a more consistent experience (i.e. you know where to find instance objects -- they're always in the same place).
>
> Does that make sense, or am I missing your point?  And if I'm
> missing your point, how would you deal with the dependency issues?

Yes, I think that makes sense; I would pose our problem as follows:

 - for Connector (and every other compontent) logging should be a
   one-shot call like `echo'

 - if we have a logger instance (vs. static class) all components need
   a way to get hold on the logger

How does a component get hold on the logger in order to do the
one-shot call to log()?

Option 1: Pass a service locator down to each component, use SL to
retrieve logger

Bad, hidden dependency.

Option 2: Pass the logger instance down to each component

Better. Question is if the logger is required or optional. If it is
optional (setter injection) we repeat

if ($this->logger) { $this->logger->log(...); }

over and over again. If it is required we bloat the constructor.

Option 2 is okayish for the logger, but for a specialized search specs
YAML loader?

Only QueryBuilder uses this class (so far); we can't tell QB to pull
it out of a ServiceLocator (=> Option 1, Bad) thus the class using QB
needs to constructor-inject the loader into the query builder. I don't
like this, it bloats the constructor. Setter-injecting is bad because
QB absolutely requires a YAML loader.

What about

public static function log()

that redirects to the configured logger instance or drops the message
with an E_NOTICE if the logger instance is not set up?

And similar for the YAML loader?

Best,
  -- David
--
David Maus
Herzog August Bibliothek - D-38299 Wolfenbuettel
Phone: +49-5331-808-317
Email: ma...@ha...

PGP Key 0x0CC2E093512F7385
Fingerprint 1AD2 EE67 224F 18C5 EA55  98AD 0CC2 E093 512F 7385