Injections on bound class #62

Closed
bachpedersen opened this Issue May 1, 2013 · 7 comments

Comments

Projects
None yet
2 participants
@bachpedersen

It looks like implementations of an interface does not get "injections"
Eg:
$container->set('Phoriz\Session\SessionHandler')
->bindTo('Phoriz\Session\FileSession\FileSessionHandler');

It looks like Phoriz\Session\SessionHandler gets it properties injected, but Phoriz\Session\FileSession\FileSessionHandler does not.

Is this by design?

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli May 1, 2013

Member

Hi,

Yes it is by design.

If you have a configuration like so:

<?php
return [

    'Phoriz\Session\SessionHandler' => [
        'class' => 'Phoriz\Session\FileSession\FileSessionHandler',
        'properties' => [
            'foo' => 1,
        ],
    ],

    'Phoriz\Session\FileSession\FileSessionHandler' => [
        'properties' => [
            'foo' => 2,
        ],
    ],

];

The first configuration is what you get for: $container->get('Phoriz\Session\SessionHandler').

The second configuration is what you get for: $container->get('Phoriz\Session\FileSession\FileSessionHandler').

Basically they are different entries.


However if you use annotations, then annotations should be taken into account on both the interface and the implementation!

If that is not the case, that is a real bug.

Let me know.

Member

mnapoli commented May 1, 2013

Hi,

Yes it is by design.

If you have a configuration like so:

<?php
return [

    'Phoriz\Session\SessionHandler' => [
        'class' => 'Phoriz\Session\FileSession\FileSessionHandler',
        'properties' => [
            'foo' => 1,
        ],
    ],

    'Phoriz\Session\FileSession\FileSessionHandler' => [
        'properties' => [
            'foo' => 2,
        ],
    ],

];

The first configuration is what you get for: $container->get('Phoriz\Session\SessionHandler').

The second configuration is what you get for: $container->get('Phoriz\Session\FileSession\FileSessionHandler').

Basically they are different entries.


However if you use annotations, then annotations should be taken into account on both the interface and the implementation!

If that is not the case, that is a real bug.

Let me know.

@bachpedersen

This comment has been minimized.

Show comment
Hide comment
@bachpedersen

bachpedersen May 1, 2013

Hi
Thanks for quick response :+1
I am using annotations, and it does not look like the annotations on the implementation is taken into account if you get the implementation using the interface like this:
$container->get('Phoriz\Session\SessionHandler')
This will not inject annotated properties in FileSessionHandler.

It worked in version 2.1 (using alias), it would be really usefull to have the alias back or the annotations taken into account on the implementation.

Hi
Thanks for quick response :+1
I am using annotations, and it does not look like the annotations on the implementation is taken into account if you get the implementation using the interface like this:
$container->get('Phoriz\Session\SessionHandler')
This will not inject annotated properties in FileSessionHandler.

It worked in version 2.1 (using alias), it would be really usefull to have the alias back or the annotations taken into account on the implementation.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli May 1, 2013

Member

OK I see, I reproduced it in tests.

I will look into that.

Member

mnapoli commented May 1, 2013

OK I see, I reproduced it in tests.

I will look into that.

@bachpedersen

This comment has been minimized.

Show comment
Hide comment
@bachpedersen

bachpedersen May 1, 2013

great, let me know if there is anything i can do to help

great, let me know if there is anything i can do to help

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli May 1, 2013

Member

The more I think about it, the more I believe you are right about aliasing.

I don't see any reason this shouldn't be a feature:

return [

    'Phoriz\Session\SessionHandler' => [
        'class' => 'Phoriz\Session\FileSession\FileSessionHandler',
    ],

    'Phoriz\Session\FileSession\FileSessionHandler' => [
        'properties' => [
            'foo' => 2,
        ],
    ],

];

By "merging" the definition of the alias into the current definition (i.e. by merging the implementation definition into the interface), that allows for simple aliasing. And that solves the current problem with annotations.

That would also allow to have the same behavior between annotations and other definition formats (array, yaml...). Everything would be simpler.

What do you think about it?

Member

mnapoli commented May 1, 2013

The more I think about it, the more I believe you are right about aliasing.

I don't see any reason this shouldn't be a feature:

return [

    'Phoriz\Session\SessionHandler' => [
        'class' => 'Phoriz\Session\FileSession\FileSessionHandler',
    ],

    'Phoriz\Session\FileSession\FileSessionHandler' => [
        'properties' => [
            'foo' => 2,
        ],
    ],

];

By "merging" the definition of the alias into the current definition (i.e. by merging the implementation definition into the interface), that allows for simple aliasing. And that solves the current problem with annotations.

That would also allow to have the same behavior between annotations and other definition formats (array, yaml...). Everything would be simpler.

What do you think about it?

mnapoli added a commit that referenced this issue May 1, 2013

@bachpedersen

This comment has been minimized.

Show comment
Hide comment
@bachpedersen

bachpedersen May 2, 2013

That sunds like a very good solution.

That sunds like a very good solution.

mnapoli added a commit that referenced this issue May 4, 2013

Refactored DI\Configuration into DI\Definition\DefinitionManager
DefinitionManager::getDefinition now merges definitions when using aliases (#62)
Cache has been moved into DefinitionManager: simpler and now caches DefinitionManager::getDefinition logic
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli May 4, 2013

Member

I released 3.0.1 with the fix for this bug.

Let me know if there's any problem.

Member

mnapoli commented May 4, 2013

I released 3.0.1 with the fix for this bug.

Let me know if there's any problem.

@mnapoli mnapoli closed this May 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment