Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Mar 28, 2016

Conversation

mnapoli
Copy link
Member

@mnapoli mnapoli commented Sep 4, 2014

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

  • I had to remove the CheckExceptionOnInvalidReferenceBehaviorPass compiler pass.

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.

  • The user now has to extend our custom Kernel class instead of Symfony's one. I don't see that as a problem, is it?
  • This branch breaks BC -> needs a major version bump
  • The docs need to be updated

Feedback more than welcome, ping @tentacode

@mnapoli mnapoli added this to the 2.0 milestone Sep 4, 2014
@mnapoli mnapoli self-assigned this Sep 4, 2014
@tentacode
Copy link

Exactly what I needed :D

@tentacode
Copy link

:shipit:

@mtotheikle
Copy link

This is awesome.

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

@mnapoli
Copy link
Member Author

mnapoli commented Sep 5, 2014

@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
Copy link
Member Author

mnapoli commented Sep 5, 2014

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
Copy link

@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
Copy link
Member Author

mnapoli commented Sep 5, 2014

@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:

  • ask users to remove Debug::enable(); from their app_dev.php
  • automatically disable DebugClassLoader in the Kernel
  • automatically disable DebugClassLoader and enable instead a replacement class that does the same except checking the class name case

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
Copy link

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 mnapoli removed their assignment May 23, 2015
@mnapoli mnapoli force-pushed the improved-interoperability branch 4 times, most recently from bb4f90d to 03b59e0 Compare March 28, 2016 08:06
@mnapoli
Copy link
Member Author

mnapoli commented Mar 28, 2016

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:

  • the CheckExceptionOnInvalidReferenceBehaviorPass is disabled because it's incompatible with PHP-DI
  • the DebugClassLoader is automatically disabled because it's incompatible with PHP-DI entry names

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.

@mnapoli mnapoli merged commit dae8978 into master Mar 28, 2016
@mnapoli mnapoli deleted the improved-interoperability branch May 15, 2016 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants