slimphp / Slim

Slim is a PHP micro framework that helps you quickly write simple yet powerful web applications and APIs.
http://slimframework.com
MIT License
11.94k stars 1.95k forks source link

Replace Pimple with a DI-Contract #1155

Closed rkrx closed 9 years ago

rkrx commented 9 years ago

Please consider replacing Pimple with something much more useful like PHP-DI or (even better) a DI-Contract to make me able to set a container by configuration. Please do NOT force me to use Pimple.

ecoreng commented 9 years ago

+1

Consider: https://github.com/container-interop/container-interop

mackenza commented 9 years ago

perhaps some further feedback into why Pimple is not a good choice would be in order? You say "able to set a container by configuration". Is that the extent of it or are there other reasons? I think @codeguy would probably appreciate some more detail here to distinguish this request from that of opinion or personal bias.

akrabat commented 9 years ago

Personally, I like Pimple for simple DI needs. When I need something more powerful, then I personally use Zend\ServiceManager - it's also required if I'm using Zend\Form. With Slim 3, this means that I have two DI containers in play at the same time which is a small nuisance as you have to either make one aware of the other or keep track of what is where. In these cases, it would be nice to be able to replace Pimple with Zend\ServiceManager.

rkrx commented 9 years ago

Just as @akrabat wrote: May be I already use another DI Container

This.

@mackenza: Is that the extent of it or are there other reasons? PHP-DI (for example) supports autowiring. That means (for me) that I don't have to ship a DI-Configuration for more then 95% of all classes involved. Autowiring is simply a technique that tries to automatically instantiate depending classes by looking at a method's (e.g. Constructor) signature and it's parameter-types (Interfaces, Abstract Classes, Classes). For interfaces and abstract classes as well as for scalar values you still have to specify by configuration what their counterpart is. That is in some way comparable to include/require vs. autoloading.

Some more?

PHP-DI would support a call-Method, with could be interesting for slim, too. So in theory you could use something like:

<?php
$app = new \Slim\Slim();
$app->get('/hello/:name', function ($name, MyService $service) {
    $salutation = $service->getSalutationForName($name);
    echo "Hello, {$salutation} {$name}";
});
$app->run();
geggleto commented 9 years ago

How will this work with older apps upgrading to 3.x ?

I really dislike in the example above the signature change, that for me personally would be an absolute reason why I would not upgrade my current projects.

codeguy commented 9 years ago

We need access to the container in the Slim\App constructor so we can register Slim's default services. This means we'll have to pass the DI container instance into the Slim\App constructor as an element of the configuration argument array. I have heard good things about Container Interop, but its interface does not explicitly accommodate our current expectation that service factory methods receive the container instance as an argument. Would this not cause problems when registering Slim's default services that assume the container is provided to each factory method as an argument if your third-party container does not do this?

codeguy commented 9 years ago

I guess we could inject the container into each factory method via use and rely on the get($id) interface method. Messy, but it solves the problem.

rkrx commented 9 years ago

@codeguy I have heard good things about Container Interop, but its interface does not explicitly accommodate our current expectation that service factory methods receive the container instance as an argument. You only have to create an extra layer for the DIC internally. Say, we have the following code:

$app->get('/hello/{name}', function ($request, $response, $args) {
    $this['view']->render('profile.html', [
        'name' => $args['name']
    ]);
})

All you need to do is to put some method in the background that activates when you access $this['view']. Could look like this:

public function offsetGet($key) {
    return $this->dic->get($key);
}

But beware. By using $this['view']->render(...) there is no chance for PHPStorm (et al) to understand, on what object render(...) is invoked. If this is the only way to access services, then you have no change to apply static analysis on the code what makes growing application harder to maintain.

Currently, you could already do something like this with PHP-DI:

// Create Slim app
$app = new \Slim\App();

// Build the DIC
$builder = new \DI\ContainerBuilder();
$builder->addDefinitions('Config/di.php');
$builder->useAnnotations(false);
$dic = $builder->build();

// Use Twig View service in route
$app->get('/hello/{name}', function ($request, $response, $args) use ($dic) {
    $dic->call(function (Slim\Views\Twig $twig) use ($args) {
        $twig->render('profile.html', [
            'name' => $args['name']
        ]);
    });
})->setName('profile');

// Run app
$app->run();

This is kind of ugly. What about this (only works with PHP-DI's make-function internally):

use Slim\Views\Twig as TwigView;

// Create Slim app
$app = new \Slim\App();

// Build the DIC
$builder = new \DI\ContainerBuilder();
$builder->addDefinitions('Config/di.php');
$builder->useAnnotations(false);
$dic = $builder->build();

// Optional step. If you omit this, the standard DIC will be used
$app->setDependencyInjectionContainer($dic);

// Use Twig View service in route
$app->get('/hello/{name}', function ($request, $response, $args, TwigView $view) {
    $view->render('profile.html', [
        'name' => $args['name']
    ]);
})->setName('profile');

// Run app
$app->run();

Or:

use Slim\Views\Twig as TwigView;

// Create Slim app
$app = new \Slim\App();

// Build the DIC
$builder = new \DI\ContainerBuilder();
$builder->addDefinitions('Config/di.php');
$builder->useAnnotations(false);
$dic = $builder->build();

// Optional step. If you omit this, the standard DIC will be used
$app->setDependencyInjectionContainer($dic);

// Use Twig View service in route
$app->get('/hello/{name}', function ($request, $response, $args) {
    $this[TwigView::class]->render('profile.html', [
        'name' => $args['name']
    ]);
})->setName('profile');

// Run app
$app->run();

But this again would break with static analysis.

@geggleto I really dislike in the example above the signature change, that for me personally would be an absolute reason why I would not upgrade my current projects. Could you provide more information about how this would break your applications?

@codeguy I guess we could inject the container into each factory method via use and rely on the get($id) interface method. Messy, but it solves the problem. Not really. Currently, no IDE would be happy about this approach. This is all duck-typing.

pmjones commented 9 years ago

@codeguy I started writing a much longer answer, but I realized that the question comes down to this: Do you actually want Dependency Injection proper, where dependencies are always injected into object from the outside? Or do you just want a Service Locator you can pass into objects so they can retrieve their own dependencies? The answer to that will determine everything else.

(For the record I am no fan of Pimple, but I can see why it appeals to you given Slim's history of using the Service Locator pattern.)

geggleto commented 9 years ago

@rkrx My apps have been written to utilize the service locator pattern (at the time it was just easier and faster).

pmjones commented 9 years ago

@geggleto Yes, Service Locator is quicker, easier, more seductive. ;-)

rkrx commented 9 years ago

@pmjones It is not necessary to have a ServiceLocator passed around everywhere. Once you have received an object (e.g. Some form of a controller) from the DIC, this object could be already instantiated with all dependencies attached and has no inner dependency to a ServiceLocator anymore. But for the Slim-Layer, this ugly ServiceLocator is the only way of accessing other objects.

@geggleto That would be no problem as long as slim won't drop the ability to access objects though that $app-container. But why did you even do that? Autowiring is much simpler and for a certain extent 'immune' to drastical dependency-changes in your application's business-objects.

pmjones commented 9 years ago

@rkrx Believe me, I know. I'm more interested in what @codeguy wants here. I'm no fan of SL, but it might be better for what @codeguy sees as the target audience for Slim.

ecoreng commented 9 years ago

Here's a repository where the Application provided overloads the pimple methods to allow the user to use a container-iterop compatible container (optionally) without having to sacrifice the definitions already in pimple (both containers coexist)

https://github.com/thecodingmachine/interop.silex.di/blob/2.0/src/Mouf/Interop/Silex/Application.php

would something like this work here?

rkrx commented 9 years ago

@ecoreng That is more or less what I've mentioned earlier. But you also have no solution for the SL-problem.

ecoreng commented 9 years ago

Your snippet basically says that the new container should replace pimple:

public function offsetGet($key) {
    return $this->dic->get($key);
}

Hoewever I think that there should be flexibility on which container to use, but leave the implementation details to the user. For instance by using this: https://github.com/thecodingmachine/interop.silex.di/blob/2.0/src/Mouf/Interop/Silex/Application.php#L66

public function offsetGet($id)
    {
        if ($this->rootContainer) {
            try {
                return $this->rootContainer->get($id);
            } catch (NotFoundException $ex) {
                // Fallback to pimple if offsetGet fails.
                return $this->pimpleContainer[$id];
            }
        } else {
            return $this->pimpleContainer[$id];
        }
    }

You are not necessarily replacing pimple, you are allowing the user to add another container to the mix.. but you would probably have to roll your own https://github.com/slimphp/Slim/blob/develop/Slim/ResolveCallable.php and other infrastructure to support your container which could also fallback to pimple just like in the snippet above..

Aside from the container-interop php Interface, there are other document that this "standard" proposes like the following: https://github.com/container-interop/container-interop/blob/master/docs/Delegate-lookup-meta.md#43-alternative-the-fallback-strategy Which could help us model a strategy to have the best of both worlds.

An obvious drawback is the added complexity of all this code to support using 2 (or more) containers.

akrabat commented 9 years ago

Implemented in #1179.