Issue - AbstractDiServiceFactory ,MvcTranslatorFactory throws Exception

[noopable]

Minimum bootstrap with Di, If Di is enabled, $serviceLocator->get('MvcTranslator'); throws
Zend\Di\Exception\RuntimeException: Invalid instantiator of type "NULL" for "Zend\I18n\Translator\TranslatorInterface".

Relative:
239f756#diff-0683436b236aeed3e48adbdbcfb17914

[Ocramius]

This should be only relative to the specific test, and not be changed in the setUp method

[noopable]

@Ocramius
Thanks.

[Ocramius]

@noopable I'd actually argue that if you use DI, this should be a DI configuration issue.

This affects any service fetched via interface, and you should probably provide either an alias to a concrete instance, or provide an instantiator for this interface.

[noopable]

@Ocramius
DiAbstractServiceFactory::canCreateServiceWithName returns true with any interface.
But the Di cannot instantiate the interface without typePreference or prepared configuration.
I think it should return false.

In this PR, TranslatorFactory checks the ServiceLocator to have 'Zend\I18n\Translator\TranslatorInterface' . And if we have used any manager of AbstractPluginManagerFactory, DiAbstractServiceFactory is injected to the ServiceLocator.
Thus if we have some di configuration, the ServiceLocator::has(any interface) returns always true.
And It can't instantiate the interface and throws exception.

I use the Di for the Domain Object only.
Do you say If anyone use the Di for any purpose, he should have configurations for all the interface?

[Ocramius]

DiAbstractServiceFactory::canCreateServiceWithName returns true with any interface.
But the Di cannot instantiate the interface without typePreference or prepared configuration.
I think it should return false.

I have instantiators for services referenced by interface. That's actually how you are supposed to wire together your dependencies (via interface). I agree on being able to discover if a type is actually instantiable via Zend\Di\Di (has a valid instantiator), but that is not implemented (hint that would actually be a good solution to the problem hint).

In this PR, TranslatorFactory checks the ServiceLocator to have 'Zend\I18n\Translator\TranslatorInterface' . And if we have used any manager of AbstractPluginManagerFactory, DiAbstractServiceFactory is injected to the ServiceLocator.
Thus if we have some di configuration, the ServiceLocator::has(any interface) returns always true.
And It can't instantiate the interface and throws exception.

Right, that is indeed annoying of the Di config, since it will likely break any ServiceManager#has() calls.

I use the Di for the Domain Object only.
Do you say If anyone use the Di for any purpose, he should have configurations for all the interface?

No, of course not, but the road is quite bumpy when using di config.
I'm just arguing that the "quickfix" provided here seems to be wrong to me.

Additionally, with this patch, the Zend\Mvc\Service\TranslatorServiceFactory and the Zend\I18n\Translator\TranslatorServiceFactory would have to be kept in sync for consistency, which is likely going to cause other bugs in future.

[noopable]

@Ocramius
Thanks.

I'm just arguing that the "quickfix" provided here seems to be wrong to me.
, which is likely going to cause other bugs in future.

I agree with you.
I compared plans in parallel that fix this problem.

First, modify AbstractDiServiceFactory::canCreateServiceWithName ex. remove $this->definitions->hasClass but it breaks instantiation from ClassName.
Second, Remove interface check from RuntimeDefinition::hasClass. I can't realize. But I guess It breaks interface detection.
Last. Redirect to Zend\I18n\Translator\TranslatorServiceFactory. Small impact but this class not to be meaningful. It is against to usability of ServiceManager.

Now, Adding Translator configuration with Di is good if we use the Di . Hmmm.

[Ocramius]

@noopable a fix would be to "try" instantiating it (hardening the logic at @239f7565e3869f49952c848c2aa52c11a2c13f7e and sacrificing performance - perfectly acceptable though)

if ($this->instanceManager->hasSharedInstance($requestedName)
    || $this->instanceManager->hasAlias($requestedName)
    || $this->instanceManager->hasConfig($requestedName)
    || $this->instanceManager->hasTypePreferences($requestedName)
) {
    return true;
}

if (! $this->definitions->hasClass($requestedName)) {
    return false;
}

try {
    $this->get($requestedName);
} catch (DiException $e) {
    return false;
}

return true;

[noopable]

@Ocramius
It works.
BTW, Why does TranslatorServiceFactory::createService get the service with the interface name?
Is it bad to use alias or service name as reserved-words, ex. MvcTranslatorAlias?

        if ($serviceLocator->has('MvcTranslatorAlias')) {
            return new MvcTranslator($serviceLocator->get('MvcTranslatorAlias'));
        }

And anyway, If someone wants to set a Translator instance of MvcTranslator, he can configure the serviceManager to use a key 'MvcTranslator' as the alias or the service instance or override factory configuration of it in module.config.php, doesn't he?

[Ocramius]

@noopable using existing class names/interface names is actually a good practice. It is very explicit and less error prone.

As for overriding the translator: yes, that indeed can be done, but I think the interface-based service name is there for BC compat.

[noopable]

Sorry, the last commit fails. (DiAbstractServiceFactoryTest)

[noopable]

@Ocramius
The interface-based service name in this factory is not for BC break. It was new released from 2.3.0.

It is good generally to use a namespace driven service name. But I prefer to use symbolic names , because we have some annoying between ServiceManager and RuntimeDefinition.
To say the least of ServiceListenerFactory::defaultServiceConfig, we use almost symbolic names.
At the point of the Framework, less problem is better,isn't it?

[Ocramius]

@noopable even in the context of Zend\Di, you should really use interface names for singleton-ish services.

I even added a note to the docs last year or so:

[noopable]

@Ocramius
Yes it is.
When I use Zend\Di and my Services, I set services with FQDN.
Can you tell me why ServiceListenerFactory::defaultServiceConfig use almost reserved-names?

[Ocramius]

@noopable because that's where those "reserved" services actually come from? :-) That's where the entire framework is glued together

[noopable]

@Ocramius

That's where the entire framework is glued together

Yes.
I say about that why does MvcTranslatorFactory check 'Zend\I18n\Translator\TranslatorInterface'?
It is just in the Framework. Use glue!

[Ocramius]

Hey @noopable, I think this PR is actually fine! I'd just want to get the tests for the abstract factory completed, if possible :-)

[noopable]

@Ocramius
Thanks.
But, I wonder if 7c7d684 is good or not at the point of side effect.
To instantiate the interface when asked 'canCreateServiceWithName', is it good?

I think that interface_exists without typePreference nor alias means "can't create service".
How about it?

if ($this->instanceManager->hasSharedInstance($requestedName)
    || $this->instanceManager->hasAlias($requestedName)
    || $this->instanceManager->hasConfig($requestedName)
    || $this->instanceManager->hasTypePreferences($requestedName)
) {
   return true;
}
// sorry updated 
if (! $this->definitions->hasClass($requestedName) || interface_exists($requestedName)) {
    return false;
}

[Ocramius]

@noopable in 99% of cases, the abstract factory is going to be asked to instantiate something via DI right after checking if the service exists.

Yes, the side-effect is acceptable, and mitigates the current behavior.

[BenjaminNolan]

I've applied this on my own project at @Ocramius's suggestion and can confirm it works perfectly. :)