PHP-DI / Symfony-Bridge

PHP-DI integration with Symfony
http://php-di.org/doc/frameworks/symfony2.html
Other
17 stars 11 forks source link

Symfony container entries can reference PHP-DI's entries #4

Closed mnapoli closed 8 years ago

mnapoli commented 10 years ago

Symfony entires can reference PHP-DI entries. This means complete interoperability between the 2 containers! For the record, PHP-DI entires can already reference Symfony's entries.

That's the solution if you need to use tags (e.g. voters, …) for a specific service -> you define it in Symfony (YAML) and you can still inject PHP-DI services into it.

For example:

services:
    myservice:
        class: My\Service
        arguments: [ "@My\Other\Service" ]
        tags:
            -  { name: mailer.transport }

Here My\Other\Service is a service that PHP-DI will provide (not Symfony).

And the whole thing is even easier than before. Currently, what you have to do:

class AppKernel extends Kernel
{
    // ...

    protected function getContainerBaseClass()
    {
        return 'DI\Bridge\Symfony\SymfonyContainerBridge';
    }

    protected function initializeContainer()
    {
        parent::initializeContainer();

        $builder = new \DI\ContainerBuilder();
        $builder->wrapContainer($this->getContainer());

        // Configure your container here
        $builder->addDefinitions(__DIR__ . '/config/config.php');

        $this->getContainer()->setFallbackContainer($builder->build());
    }
}

Now with this branch, you only do this:

class AppKernel extends \DI\Bridge\Symfony\Kernel
{
    // ...

    protected function buildPHPDIContainer(ContainerBuilder $builder)
    {
        // Configure your container here
        $builder->addDefinitions(__DIR__ . '/config/config.php');

        return $builder->build();
    }
}

Warnings

It checked for unknown references, and since it doesn't know about PHP-DI's entries, they are "unknown references" to it, so it throws exceptions.

I tried to replace it with a custom compiler pass (that would look into PHP-DI too), but PHP-DI is not built yet when Symfony's container is being compiled, so it doesn't seem really possible.

So is that really bad to have removed that compiler pass? Is that worth it? To be discussed.

Feedback more than welcome, ping @tentacode

tentacode commented 10 years ago

Exactly what I needed :D

tentacode commented 10 years ago

:shipit:

mtotheikle commented 10 years ago

This is awesome.

Just to clarify, the at symbol indicates this is a class name and will be coming from PHP DI container?

mnapoli commented 10 years ago

@mtotheikle the @ symbol means (in Symfony's container) that this is a reference to another container entry (the same as DI\link() in PHP-DI). So the entry can be either in PHP-DI or in Symfony.

Here in my example it's a class name, and since PHP-DI uses "autowiring", container entries are class names (because they are auto-detected, but it's not mandatory at all). So yes, in that example the dependency will be provided by PHP-DI.

To be more explicit:

# Symfony
services:
    foo:
        class: My\Service
        arguments: [ "@bar", "@abc" ]
    bar:
        class: My\Other\Service
<?php
return [
    'abc' => DI\object('Some\Other\Class'),
];

bar will come from Symfony, abc from PHP-DI. abc was explicitly defined in PHP-DI under a custom name (abc) instead of a class name, but whatever, the name can be anything.

mnapoli commented 10 years ago

I have added a demo/ folder containing an installation of the standard edition. I think I get the same error as you might have seen @tentacode:

Case mismatch between loaded and declared class names: acme\demobundle\service\barservice vs Acme\DemoBundle\Service\BarService

The config:

services:
    foo:
        class: Acme\DemoBundle\Service\FooService
        arguments: [ "@Acme\DemoBundle\Service\BarService" ]

It seems Symfony's container lowercases every entry name, and then when PHP-DI tries to load acme\demobundle\service\barservice, the DebugClassLoader of Symfony (btw where does this gets registered??) throws an exception because it doesn't like the lower case class name…

I'm still looking for a solution…

mtotheikle commented 10 years ago

@mnapoli I totally overlooked that the at symbol was indicating to Symfony that that variable is a service. I've worked with Symfony YAML configuration a lot and when I saw your first comment it just threw me off cause I was thinking it was special to PHP-DI as I was trying to figure out the specific changes you made to make this possible.

The DebugClassLoader gets enabled by calling Symfony\Component\Debug\Debug::enable(..) in most systems, which to me means that this is going to require some sort of PR to Symfony

mnapoli commented 10 years ago

@mtotheikle thanks for the help, this is indeed that.

If I comment out Debug::enable(); in app_dep.php everything works.

So we have 3 options:

The 3rd option seems more practical but it means a class to maintain in sync with Symfony.

Doing a PR to Symfony will take too much time, and it will probably never get accepted as long as it doesn't benefits Symfony itself. I don't want to get my hopes up on this :/

What do you guys think?

eddiejaoude commented 9 years ago

My 2 cents:

  1. ask users to remove Debug::enable(); from their app_dev.php

I don't really like this option - what functionality/transparency is lost by removing this?

  1. automatically disable DebugClassLoader in the Kernel

what functionality/transparency is lost by removing this?

  1. automatically disable DebugClassLoader and enable instead a replacement class that does the same except checking the class name case

This seems like the better option

mnapoli commented 8 years ago

I've rebased everything and I'm planning this for the 2.0 release (which will require PHP-DI 5 and Symfony 3).

Conclusion about the issues mentioned in this thread:

Both are not serious issues, only tools for development. I will implement a workaround for the first one to avoid losing the feature. For the second, I don't see one for now, but it shouldn't be a problem it's just a helper for debugging.

Merging on master, I'll tag a beta version today.