Skip to content
This repository has been archived by the owner on Feb 6, 2020. It is now read-only.

Conversation

thomasvargiu
Copy link
Contributor

@thomasvargiu thomasvargiu commented Feb 1, 2017

Trying to resolve missed cache using ReflectionBasedAbstractFactory generating config mappings for a whole module/project.

Skipping classes with unresolved dependencies

Added ability to ignore only classed with direct unresolved dependencies in ConfigDumper command.

At this moment, generate-deps-for-config-factory command throws an exception if just one dependency in the the dependency tree could not be resolved.

Adding a parameter -i|--ignore-unresolved it will not add the definition of the class with an unresolved dependency, but continuing the execution adding other dependency definitions.

Consider these classes:

namespace TestAsset;

class ObjectWithObjectScalarDependency
{
    public function __construct(
        SimpleDependencyObject $simpleDependencyObject,
        ObjectWithScalarDependency $dependency,
        TestInterface $interface
)  {
    }
}

class SimpleDependencyObject
{
    public function __construct(InvokableObject $invokableObject)
    {
    }
}

class ObjectWithScalarDependency
{
    public function __construct($aName)
    {
    }
}

class InvokableObject
{
    public function __construct(array $options = [])
    {
        $this->options = $options;
    }
}

interface TestInterface
{
}

Dependency tree:

+ ObjectWithObjectScalarDependency
|
+---> SimpleDependencyObject
|     |
|     +---> InvokableObject
|
+---> ObjectWithScalarDependency
|     |
|     +---> $aName (scalar)
|
+---> TestInterface (interface)

Command:

$ generate-deps-for-config-factory ./config.php TestAsset\ObjectWithObjectScalarDependency

This command will write nothing. An exception will throw.

Command:

$ generate-deps-for-config-factory --ignore-unresolved ./config.php TestAsset\ObjectWithObjectScalarDependency

This command will generate a config with the following mapping config:

$config = [
    ObjectWithObjectScalarDependency::class => [
        SimpleDependencyObject::class,
        ObjectWithScalarDependency::class,
        TestInterface:: class,
    ],
    SimpleDependencyObject::class => [
        InvokableObject::class,
    ],
    InvokableObject::class => [],
];

Like for interfaces, classes with unresolved dependencies will be included in other classes dependencies, but no definitions will be written.

This will be useful to generate something similar a cache for a whole project/module.

The idea is to do something like this:

$ find ./module/Auth/src/ -name "*.php" | xargs ./bin/classes-from-file | xargs ./bin/generate-deps-for-config-factory ./config.php

ContainerInterface dependecy in ConfigDumper

I added the ability to inject a ContainerInterface in the ConfigDumper class.

It's used in ConfigDumper::createDependencyConfig().

If a container is injected, before to throw an exception, it checks if the class with an unresolved dependency is already defined in the container. In this case we can safely skip the dependency definition for this class.

Considerations

I preferred to don't add a command to get classes in files (scanning a directory) to don't include another dependency in this project.

@thomasvargiu thomasvargiu changed the base branch from master to develop February 1, 2017 00:34
Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fantastic improvement! I consider it a "bugfix", though; while it's adding some new functionality, that functionality fixes a perceived flaw with the command. As such, I'll cherry-pick it to the master branch.

I'll also incorporate the feedback I've made, as it's all trivial.

@@ -26,12 +27,34 @@ class ConfigDumper
EOC;

/**
* @var ContainerInterface
*/
protected $container;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private visibility, please.

/**
* @param ContainerInterface $container
*/
public function setContainer(ContainerInterface $container)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this setter really necessary? If it is, then use it from __construct().

<?php
/**
* @link http://github.com/zendframework/zend-servicemanager for the canonical source repository
* @copyright Copyright (c) 2016 Zend Technologies USA Inc. (http://www.zend.com)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2017, not 2016. 😄

@weierophinney weierophinney added this to the 3.2.1 milestone Feb 15, 2017
weierophinney added a commit that referenced this pull request Feb 15, 2017
weierophinney added a commit that referenced this pull request Feb 15, 2017
@weierophinney
Copy link
Member

Thanks, @thomasvargiu!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants