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

Broken ServiceManager #11

Closed
aeneasr opened this issue Jan 17, 2014 · 17 comments
Closed

Broken ServiceManager #11

aeneasr opened this issue Jan 17, 2014 · 17 comments

Comments

@aeneasr
Copy link

aeneasr commented Jan 17, 2014

I took a closer look into ZendDiCompiler but I still got problems to get it working. As stated in the docs, the Container will be added as a fallback to the ServiceManager. However, here is an example config:

    'view_manager'          => array(
        'strategies'               => array(
            'Ui\Strategy\PhpRendererStrategy'
        )
    ),
    'service_manager'       => array(
        'factories'  => array(
            'Ui\Renderer\PhpDebugRenderer' => function (ServiceLocatorInterface $sm) {
                    $service = new Renderer\PhpDebugRenderer();
                    $service->setResolver($sm->get('Zend\View\Resolver\AggregateResolver'));
                    $service->setHelperPluginManager($sm->get('ViewHelperManager'));

                    return $service;
                },
        )
    ),

So the PhpRendererStrategy should be known to the default ServiceManager. However, when I activate the ZDC in my application.config with the following settings:

    'zendDiCompiler'     => array(
        // nothing to scan yet
    ),

I get the following exceptions:

Fatal error: Uncaught exception 'Zend\ServiceManager\Exception\ServiceNotCreatedException' with message 'The factory was called but did not return an instance.' in /var/www/src/vendor/zendframework/zendframework/library/Zend/ServiceManager/ServiceManager.php on line 904

Zend\ServiceManager\Exception\ServiceNotCreatedException: An abstract factory could not create an instance of uistrategyphprendererstrategy(alias: Ui\Strategy\PhpRendererStrategy). in /var/www/src/vendor/zendframework/zendframework/library/Zend/ServiceManager/ServiceManager.php on line 1075

I thought that this might be related to the ViewManager, however, if I deactivate that view strategy, other services can't be instantiated as well (same error). Also it doesn't matter where I place ZDC in the module section.

One more thing:

Allows for custom code introspection strategies (by default, only constructors are scanned)

This has to do directly with DI and not ZDC right? So if I do this:


    'di'             => [
        'definition'          => [
            'class' => [
                'Discussion\DiscussionManager'                => [
                    'setObjectManager'        => [
                        'required' => true
                    ],
                ],
                // ...

The method setObjectManager will be scanned as well, won't it?

@aeneasr
Copy link
Author

aeneasr commented Jan 17, 2014

This could happen, because of

    /**
     * Determine if we can create a service with name.
     *
     * This function is called by the ServiceManager.
     *
     * @param ServiceLocatorInterface $serviceLocator
     * @param string                  $name
     * @param string                  $requestedName
     *
     * @return bool
     */
    public function canCreateServiceWithName(ServiceLocatorInterface $serviceLocator, $name, $requestedName)
    {
        return true; // Yes, we can!
    }

I'm not really familiar with the internals of ZDC yet, but it wouldn't probably hurt to check if our factory actually could instantiate that. Maybe the issue is related to that - but like I said I'm not familiar with the internals yet :D

@aeneasr
Copy link
Author

aeneasr commented Jan 18, 2014

Also, dependencies don't get injected yet:

    public function getBlogManagerBlogManager(array $params = array(), $newInstance = false)
    {
        if (!$newInstance && isset($this->services['Blog\Manager\BlogManager'])) {
            return $this->services['Blog\Manager\BlogManager'];
        }

        $object = new \Blog\Manager\BlogManager();
        if (!$newInstance) {
            $this->services['Blog\Manager\BlogManager'] = $object;
        }

        return $object;
    }

module.config.php

    'di'             => [
        'definition'          => [
            'class' => [
                __NAMESPACE__ . '\Manager\BlogManager'       => [
                    'setTaxonomyManager'      => [
                        'required' => true
                    ],
                    'setClassResolver'        => [
                        'required' => true
                    ],
                    'setObjectManager'        => [
                        'required' => true
                    ],
                    'setUuidManager'          => [
                        'required' => true
                    ],
                    'setLanguageManager'      => [
                        'required' => true
                    ],
                    'setAuthorizationService' => [
                        'required' => true
                    ]
                ]
            ]
        ],
        'instance'            => [
            'preferences' => [
                __NAMESPACE__ . '\Manager\BlogManagerInterface' => __NAMESPACE__ . '\Manager\BlogManager'
            ]
        ]
    ],

@aeneasr
Copy link
Author

aeneasr commented Jan 18, 2014

I've also noticed, that $instance = $this->generatedServiceLocator->get($name, $params, $newInstance); requests the instances normalized (e.g. blogmanagerblogmanager), however the GeneratedServiceLocator only knows the full class names: Blog\Manager\BlogManager. Therefore, no service can be found at all!

A quick and dirty fix for canCreateServiceWithName I came up with was:

    public function canCreateServiceWithName(ServiceLocatorInterface $serviceLocator, $name, $requestedName)
    {
        try{
            return $this->get($name) !== null;
        } catch (RuntimeException $e){
            return false; // not initialized yet
        }
        //return true; // Yes, we can!
    }

I am on zf2 2.2*

@aeneasr
Copy link
Author

aeneasr commented Jan 18, 2014

So, the ServiceManager canoncializes every name, that's why the GeneratedServiceLocator doesn't work:

Zend\ServiceManager\ServiceManager:

    public function get($name, $usePeeringServiceManagers = true)
    {
        // inlined code from ServiceManager::canonicalizeName for performance
        if (isset($this->canonicalNames[$name])) {
            $cName = $this->canonicalNames[$name];
        } else {
            $cName = $this->canonicalizeName($name);
        }

@aeneasr
Copy link
Author

aeneasr commented Jan 18, 2014

Okay, so I've managed to get the GeneratedServiceLocator working. However, Injections still don't work - but I'll try to figure that one out as well.

Please note that those changes are SUPERDUPER DIRTY. I just want to point out why stuff isn't working the way it should!!

ZendDiCompiler\Generator.php: https://gist.github.com/arekkas/8490905
Added: $canonicalNamesReplacements
Modified method: createCaseStatements()

ZendDiCompiler\ZendDiCompiler.php: https://gist.github.com/arekkas/8490922
Modified method: canCreateServiceWithName()

@aeneasr
Copy link
Author

aeneasr commented Jan 18, 2014

Ok, found another one, the DI preferences can be precerences and preference

    public function setConfig(Config $config)
    {
        $this->config = $config;

        // Provide easy access to type preferences
        /** @var Config $typePreferences */
        $typePreferences       = $this->config->di->instance->preference->toArray();
        $typePreferences       = array_merge(typePreferences, $this->config->di->instance->preferences->toArray());
        $this->typePreferences = $typePreferences;
    }

@aeneasr
Copy link
Author

aeneasr commented Jan 18, 2014

Also preferences should be canonicalized as well:

Again: super dirty - just pointing out

ZendDiCompiler

    public function setConfig(Config $config)
    {
        $this->config = $config;

        // Provide easy access to type preferences
        /** @var Config $typePreferences */
        $typePreferences = $this->config->di->instance->preferences;
        $canonicalized   = [];
        foreach($typePreferences->toArray() as $key => $value){
            $key = strtolower(strtr($key, $this->canonicalNamesReplacements));
            $value = strtolower(strtr($value, $this->canonicalNamesReplacements));
            $canonicalized[$key] = $value;
        }
        $this->typePreferences = $canonicalized;
    }

And the get method should canonicalize by default:

ZendDiCompiler

    public function get($name, array $params = array(), $newInstance = false)
    {
        $this->checkInit();

        $name = strtolower(strtr($name, $this->canonicalNamesReplacements));

@aeneasr
Copy link
Author

aeneasr commented Jan 18, 2014

I hope this issue doesn't get too messy, I'm just trying to figure out why the module doesn't work for me :)

@aeneasr
Copy link
Author

aeneasr commented Jan 18, 2014

The Definition in the CodeGenerator for BlogManager are empty:

object(Zend\Di\ServiceLocator\GeneratorInstance)[1923]
  protected 'class' => string 'Blog\Manager\BlogManager' (length=24)
  protected 'alias' => null
  protected 'constructor' => string '__construct' (length=11)
  protected 'params' => 
    array (size=0)
      empty
  protected 'methods' => 
    array (size=0)
      empty
  protected 'shared' => boolean true

@aeneasr
Copy link
Author

aeneasr commented Jan 18, 2014

Setting a different IntrospectionStrategy doesn't help much :/

    public function getServiceConfig()
    {
        // Instance must be created here already but is set up later.
        $this->zendDiCompiler = new ZendDiCompiler;
        $introspectionStrategy = new IntrospectionStrategy();
        $this->zendDiCompiler->setIntrospectionStrategy($introspectionStrategy);

        return array(
            // Set zendDiCompiler as fallback. Now Zend\ServiceManager uses ZendDiCompiler to retrieve instances.
            'abstract_factories' => array(
                $this->zendDiCompiler,
            ),
            // Provide ZendDiCompiler as a Zend\ServiceManager service.
            'services' => array(
                'ZendDiCompiler' => $this->zendDiCompiler,
            )
        );
    }

@aimfeld
Copy link
Owner

aimfeld commented Jan 19, 2014

Thanks for your input and thorough analysis. As far as I can tell, there are three different problems here:

  1. When retrieving instances via the ServiceManager (which uses ZendDiCompiler as an abstract factory), class names get canonicalized and are therefore not retrievable by the ZendDiCompiler. I always pull instances from the ZendDiCompiler directly, so I haven't seen this problem. I try to fix this today.
  2. Setter injection does not work. The default introspection strategy supports only constructor injection. But I think this could be done. In the meantime, I have added a caveat section in the readme mentioning this limitation. Also, I have opened a separate issue for setter injection support.
  3. ZendDiCompiler asserts that it can create any instance in canCreateServiceWithName , even if that's not true. As ZendDiCompiler is used as a last resort (fallback), when pulling instances from the ServiceManager, I thought it was acceptable to let it fail. But your fix is a clever idea, I will add it today to the master!

@aeneasr
Copy link
Author

aeneasr commented Jan 19, 2014

Appreciate your work! :)

@aimfeld
Copy link
Owner

aimfeld commented Jan 19, 2014

As I have previously thought, it's probably better to not integrate ZendDiCompiler as a fallback abstract factory in the ServiceManager. It's clearer to create instances directly with the ZendDiCompiler, where you expect ZendDiCompiler to be able to create instances.

The suggested fix of the canCreateServiceWithName() method is not such a good idea after all. Before a costly re-scan of the code, it's not easy for ZendDiCompiler to determine whether a class can be instantiated without actually trying (as you do in your fix). If instantiation is not possible, the code is re-scanned (costly process) to update the GeneratedServiceLocator. The ServiceManager calls canCreateServiceWithName() for many ZF2-specific classes which you usually don't want to create with ZendDiCompiler (e.g. the ModuleManager instance). This will lead to a code re-scan at every request, slowing things down way too much.

As we see, there are also problems with canonical class names and I suspect other problems with class aliasing, too. Therefore, I will update ZendDiCompiler to not register itself as a fallback abstract factory for the ServiceManager. Note that it's still easy to use ZendDiCompiler via the ServiceManager. You can just write a ServiceManager callback which uses ZendDiCompiler to create an instance. That's what I do when I create e.g. controllers, controller plugins, or view helpers. Here's an example how I create view helpers:

public function getViewHelperConfig()
{
    return array(
        'factories' => array(
            'image'          => function () {
                return $this->zendDiCompiler->get('Survey\View\Helper\Image');
            },
        ),
    );
}

@aeneasr
Copy link
Author

aeneasr commented Jan 19, 2014

The suggested fix of the canCreateServiceWithName() method is not such a good idea after all. Before a costly re-scan of the code, it's not easy for ZendDiCompiler to determine whether a class can be instantiated without actually trying (as you do in your fix). If instantiation is not possible, the code is re-scanned (costly process) to update the GeneratedServiceLocator. The ServiceManager calls canCreateServiceWithName() for many ZF2-specific classes which you usually don't want to create with ZendDiCompiler (e.g. the ModuleManager instance). This will lead to a code re-scan at every request, slowing things down way too much.

Yes I thought so.

Did you check OcramiusDiCompiler (https://github.com/Ocramius/OcraDiCompiler) ? I think he replaces the default DI Factory and makes it therefore still possible to use the compiled service manager alongside with the default one. However, there were problems with the getter/setter injection as well ( see Ocramius/OcraDiCompiler#7 )

aimfeld pushed a commit that referenced this issue Jan 19, 2014
…iceManager anymore. It creates too many problems.
@aimfeld
Copy link
Owner

aimfeld commented Jan 19, 2014

I have been in contact with Ocramius, ZendDiCompiler emerged as an alternative to the OcraDiCompiler which didn't really work for me. I have not thoroughly studied his code though. There may be cool ideas in there which could be integrated in ZendDiCompiler.

Even without registering ZendDiCompiler as a fallback abstract factory for the ServiceManager, it's still easy to use both alongside, as mentioned. The fallback option was more of a nice-to-have feature, which created more problems than it solves.

@aeneasr
Copy link
Author

aeneasr commented Jan 19, 2014

Ah I see. :)

I'll check out using the diCompiler in the module.php some time soon :)

aimfeld pushed a commit that referenced this issue Jan 19, 2014
@aimfeld
Copy link
Owner

aimfeld commented Jan 19, 2014

I have created a new tag 1.1.8 which does not register the ZendDiCompiler as a fallback abstract factory anymore. The setter injection problem will be tackled in a separated issue

@aimfeld aimfeld closed this as completed Jan 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants