PHP-DI / Symfony-Bridge

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

Unable to reference PHP DI dependencies in Symfony 3.1 #16

Open pevdh opened 8 years ago

pevdh commented 8 years ago

First of all, thank you for this very useful bridge!

I am trying to use the Symfony bridge with Symfony 3.1, but I ran into an issue where I seem to be unable to reference to PHP DI dependencies from the Symfony services.yml configuration file.

This is my services.yml configuration file (as suggested in the docs):

services:
    app.page_layout_extension:
        class: AppBundle\Twig\PageLayoutExtension
        arguments:
            - 'AppBundle\Layout\PageLayoutFactory'
            - 'AppBundle\Service\ContentBlockService'
        tags:
            - {name: twig.extension}

The signature of the PageLayoutExtension constructor is public function __construct(PageLayoutFactory $pageLayoutFactory, ContentBlockService $contentBlockService)

This configuration yields the following error:

PHP Fatal error:  Uncaught Symfony\Component\Debug\Exception\FatalThrowableError: Type error: Argument 1 passed to AppBundle\Twig\PageLayoutExtension::__construct() must be an instance of AppBundle\Layout\PageLayoutFactory, string given, called in /var/cache/dev/appDevDebugProjectContainer.php on line 300 in /src/AppBundle/Twig/PageLayoutExtension.php:23
Stack trace:
#0 /var/cache/dev/appDevDebugProjectContainer.php(300): AppBundle\Twig\PageLayoutExtension->__construct('AppBundle\\Layou...', 'AppBundle\\Servi...')
#1 /vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php(275): appDevDebugProjectContainer->getApp_PageLayoutExtensionService()
#2 /vendor/php-di/symfony-bridge/src/SymfonyContainerBridge.php(93): Symfony\Component\DependencyInjection\Container->get('app.page_layout...', 1)
#3 /vendor/php-di/symfony-bridge/src/SymfonyContainer in /src/AppBundle/Twig/PageLayoutExtension.php on line 23

Upon inspection, this error is caused by this line in appDevDebugContainer.php:

return $this->services['app.page_layout_extension'] = new \AppBundle\Twig\PageLayoutExtension('AppBundle\\Layout\\PageLayoutFactory', 'AppBundle\\Service\\ContentBlockService');

Which makes the error obvious. This line should read:

return $this->services['app.page_layout_extension'] = new \AppBundle\Twig\PageLayoutExtension($this->get('AppBundle\\Layout\\PageLayoutFactory'), $this->get('AppBundle\\Service\\ContentBlockService'));

Looking into the issue, I came across the following suggested modification of services.yml (forgot where I read it):

services:
    app.page_layout_extension:
        class: AppBundle\Twig\PageLayoutExtension
        arguments:
            - '@AppBundle\Layout\PageLayoutFactory'
            - '@AppBundle\Service\ContentBlockService'
        tags:
            - {name: twig.extension}

However, this makes that same appDevDebugContainer.php line translate to:

return $this->services['app.page_layout_extension'] = new \AppBundle\Twig\PageLayoutExtension($this->get('appbundle\layout\pagelayoutfactory'), $this->get('appbundle\service\contentblockservice'));

(Note the lowercase service names). Which is clearly wrong as well.

Am I missing something or is this new "functionality" of Symfony's container? Are there any workarounds?

mnapoli commented 8 years ago

First of all yes you have to use @ in front of service names, it's the Symfony convention for saying "this is a service". Else you are simply injecting a string. It's like using DI\get() in PHP-DI configuration, if you don't use that then you will simply be injecting a string, not the service, and that's not what you want.

So the second example sounds correct.

Now yes in Symfony all service IDs are turned to lower case. I don't remember that being a problem because in PHP class names are case insensitive, and it should be covered by the tests of this package.

So in the end did the last example work? If not, what error did you get?

mnapoli commented 8 years ago

See here: https://github.com/PHP-DI/Symfony-Bridge/blob/master/tests/FunctionalTest/Fixtures/config/class1.yml This test uses the same notation as you.

And I just realize the documentation is wrong so that's maybe what mislead you: http://php-di.org/doc/frameworks/symfony2.html#using-symfonys-services-in-php-di

You can also inject PHP-DI services in a Symfony service:

services:
    # app.user_service is a Symfony service
    app.user_service:
        class: 'AppBundle\\Service\\UserService'
        arguments:
            # UserRepository is created by PHP-DI
            - 'AppBundle\\Service\\UserRepository'

The @ is missing. I will add it!

pevdh commented 8 years ago

Hmm, your suggestion about the case sensitivity is correct, I think. However, I still get You have requested a non-existent service "appbundle\service\contentblockservice", sometimes. It seems random. Might have something to do with the cache, but I do not have the time to investigate it :/

Thanks for the quick update of the documentation!

Stack trace:

ServiceNotFoundException in SymfonyContainerBridge.php line 90:
You have requested a non-existent service "appbundle\service\contentblockservice".

in SymfonyContainerBridge.php line 90
at SymfonyContainerBridge->get('appbundle\service\contentblockservice') in appDevDebugProjectContainer.php line 300
at appDevDebugProjectContainer->getApp_CmsExtensionService() in Container.php line 275
at Container->get('app.cms_extension', '1') in SymfonyContainerBridge.php line 72
at SymfonyContainerBridge->get('app.cms_extension') in appDevDebugProjectContainer.php line 3119
at appDevDebugProjectContainer->getTwigService() in Container.php line 275
at Container->get('twig', '1') in SymfonyContainerBridge.php line 72
at SymfonyContainerBridge->get('twig') in appDevDebugProjectContainer.php line 3376
at appDevDebugProjectContainer->getWebProfiler_Controller_ProfilerService() in Container.php line 275
at Container->get('web_profiler.controller.profiler', '1') in SymfonyContainerBridge.php line 72
at SymfonyContainerBridge->get('web_profiler.controller.profiler') in classes.php line 2974
at ControllerResolver->createController('web_profiler.controller.profiler:panelAction') in classes.php line 2424
at ControllerResolver->getController(object(Request)) in TraceableControllerResolver.php line 58
at TraceableControllerResolver->getController(object(Request)) in HttpKernel.php line 136
at HttpKernel->handleRaw(object(Request), '1') in HttpKernel.php line 68
at HttpKernel->handle(object(Request), '1', true) in Kernel.php line 177
at Kernel->handle(object(Request)) in app_dev.php line 30
at require('/web/app_dev.php') in router_dev.php line 40
mnapoli commented 8 years ago

Mmh if it's "sometimes" then it might be a cache issue indeed, or maybe related to a specific machine? Anyway if you can isolate a specific problem feel free to reopen an issue, the best thing is to have a reproducible test case or something similar.

Fedott commented 8 years ago

Hi!

I've faced the same problem and have figured out why it is happening.

The current test gives us false-positive result. It happens because the class names in PHP are case-insensitive, so class_exists('camelcaseclass') returns true if CamelCaseClass exists. But if we use autoloader and the appropriate class has not been loaded before calling class_exists, than class_exists('camecaseclass') will return false because of unix filesystem case-sensivity (it will look for the file called "camelcaseclass.php", which is actually missing, instead of the correct "CameCaseClass.php").

Thereby, when we use the class name in the dependencies of the service defined in the Symfony container, the dependency id is strtolower'ed. That causes the autoloader to look for the incorrect file when the php-di tries to resolve the dependency.

There are two ways of making the test work properly. The first is moving the test itself (symfonyGetInPHPDI) to the very top of the TestCase file. The second is to use different classes in the tests.

mnapoli commented 8 years ago

Ohh that's a nasty bug, thanks @Fedott for the details. I'm reopening this, I won't have time to look at this this week-end but feel free to send a pull request, even if it's just to add a test that reproduces the problem it will be useful.