factories use container as service locator and uses classnames as keys..

[ppetermann]

I just stumbled over the middleware and was surprised that the docs said it supports psr-11, wondering what it would load through it.

A quick look at the code revealed several issues:

• the factory classes all use the container in a ServiceLocator anti-pattern, which is discouraged in psr-11 section.13.
  • the identifier within the container is a classname, a better way would be to use an interface here, and then have the classes implementing the service stored for that interface.
  • for configuration the key "config" is used, which might conflict with the config of some frameworks, furthermore, there is no clear interface, o and even worse the provided ConfigProvider is final, and loads static confic files from its own config dir, that are meant for a specific framework, thus making it a necessity to replace the not-mentioned interfaceless config provider with your own.

[snapshotpl]

@ppetermann thanks your feedback

1. As injecting container into your classes is anti-pattern - that's usage in this package is absolutely right. Provided factories exists on "container" level - can be use to build object using object, following container implementation. See how zend-servicemanager, pimple create services - by invokable objects. There isn't any other object where container is injected.
2. Can you provide example?
3. config key - you have right. It can be done better. However what is problem with final class and loading config from file? Provided config is just default set of data - you should get it and modify by your needs.

[ppetermann]

1) injection of the container into the factories

I just realized from what you wrote, and from looking at how you suggest using this with slim, that you don't intent to use the factory as a registered service, but that the factory is the registration of the service it provides (meaning, it replaces the function () {return new Service();}), and that is why you inject the container where you do (as that is how pimple can deal with it) So you are right, the use is OK, I was mislead by just reading the Installation part, and skipping the parts about frameworks that I currently don't use.

2) example for interface rather than classname

Sure

• components should depend on the public interface of the service they use, not on the implementation
• the goal of IoC, which DI is a solution for is to decouple the implementation from the use of a component, so you end up with only depending on the interface
• decoupling is done so it is easy to replace parts

say you use a key like JavascriptRenderer::class:

<?php
// set it
$container[JavascriptRenderer::class] = function($c) { return new JavascriptRenderer();}

// so when you use it somewhere you'd do:
$container[PhpDebugBarMiddleware::class] = function ($c) {
  return new PhpDebugBarMiddleware($container->get(JavascriptRenderer::class));
}

class PhpDebugBarMiddleware 
{
  public function __construct(JavacscriptRenderer $debugbarRenderer)
  {
  }
}

So have you really decoupled from Javascript renderer here? nope, because your Middleware is still expecting an implementation of JavascriptRenderer, so replacing it would mean to load a class of the exact same name, and prevent the one of PhpDebugBar from loading.

now if PhpDebugBar was a well designed Software, it would provide a RendererInterface, which sadly, it is not, so for solving the dependency we should clean this up by

<?php
interface RendererInterface 
{
  public function render(); // i know there is more, but lets keep the example short
}

class WrappedJavascriptRenderer implements RendererInterface
{
  private $delegate;
  public function __construct()
  {
    // we could inject this too, but since this is the replaceable component, no point in that
    $this->delegate = new JavascriptRenderer();
  }

  public function render()
  {
    return $this->delegate->render();
  }
}

Now instead of depending on JavascriptRenderer, the middleware can depend on a replaceable component whose interface is known

<?php
class PhpDebugBarMiddleware 
{
  public function __construct(RendererInterface $debugbarRenderer)
  {

  }
}

so from this moment on the PhpDebugMiddleware would be actually decoupled, now any class implementing the interface could be used to replace JavascriptRenderer

but wait! there is more! its still using the classname for lookups, which means either you have to register a replacing component under the classname of another component, or you replace the key by the interface name, which means you will consistently use the interface name everywhere, and have only one place where the actual classname of the implementation is used: the place where the Implementing component is registered to the dependency injection

<?php
$container[RendererInterface::class] = function($c) { return new WrappedJavascriptRenderer();}

$container[PhpDebugBarMiddleware::class] = function ($c) {
  return new PhpDebugBarMiddleware($container->get(RendererInterface::class));
}

... which leaves you with a lot cleaner setup, where things are easy to replace, while keeping a consistent and correct name. Bonus Points: if you do that, there are containers out there which can autowire typehint to key in container, thus making it a bit less boilerplate for those who work with them, while still retaining all benefits.

I've left out namespaces, and i might have typos in the code above. however if i forgot anything else, don't hesitate to ask

3) config "service", and ConfigProvider

Yeah, i think for ConfigProvider i fell into the same trap as in 1), I didn't notice it was Zend specific. However having the ambigous "config" key, witht he requirement of it being an array (well i guess ArrayAccess would do) might conflict with what some frameworks do, and isn't a very clean interface either (I still hope the FIG folks get of their asses, and start putting a few config interfaces up, but as it stands every framework does their own thing here..))