Private properties of parent classes not injected properly by Container::injectOn()? #257

Closed
holtkamp opened this Issue May 26, 2015 · 11 comments

Comments

Projects
None yet
2 participants
@holtkamp
Contributor

holtkamp commented May 26, 2015

When using Modules in a ZF1 project, it is common to have controllers like this.

class Default_OrderController 
{
    /**
     * @inject
     * @var \Project\Domain\Service\Order
     */
    private $orderDomainService

    public function doSomething()
    {
        $this->orderDomainService->doSomeBusinessStuff();
    }
}

class ModuleX_OrderController extends Default_OrderController
{
    //empty on purpose
}

Using the $this->orderDomainService property works for the default Module, but not for the other Modules. It seems that the current ZF1 integration of the DIC does not inject the values of private properties of parent Controllers / classes. When changing the access level of the property to 'protected', the property does get injected...

Is this intended behaviour of \DI\Container::injectOn() used by the bridge? It would be nice if the class hierarchy upstream is taken into account as well...

PS: offtopic: still looking for a way to get rid of these 'empty' controllers, but seems not possible in ZF1

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli May 26, 2015

Member

If the property is private in the parent class, then it can't be accessed in child classes (it's just a limitation of PHP here). Does it answer your question?

Member

mnapoli commented May 26, 2015

If the property is private in the parent class, then it can't be accessed in child classes (it's just a limitation of PHP here). Does it answer your question?

@mnapoli mnapoli added the support label May 26, 2015

@holtkamp

This comment has been minimized.

Show comment
Hide comment
@holtkamp

holtkamp May 26, 2015

Contributor

yeah, I understand that. But as you can see the method exists 'in' the
parent class, it also is executed 'there', however, the current approach
does not injects the value in the property of the parent class.

Summarizing: the class hierarchy of a class (in this case a controller)
seems not respected by the injectOn().

Maybe check for existence of parents and also inject them...
On May 26, 2015 12:46 PM, "Matthieu Napoli" notifications@github.com
wrote:

If the property is private in the parent class, then it can't be accessed
in child classes (it's just a limitation of PHP here). Does it answer your
question?


Reply to this email directly or view it on GitHub
#257 (comment).

Contributor

holtkamp commented May 26, 2015

yeah, I understand that. But as you can see the method exists 'in' the
parent class, it also is executed 'there', however, the current approach
does not injects the value in the property of the parent class.

Summarizing: the class hierarchy of a class (in this case a controller)
seems not respected by the injectOn().

Maybe check for existence of parents and also inject them...
On May 26, 2015 12:46 PM, "Matthieu Napoli" notifications@github.com
wrote:

If the property is private in the parent class, then it can't be accessed
in child classes (it's just a limitation of PHP here). Does it answer your
question?


Reply to this email directly or view it on GitHub
#257 (comment).

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli May 26, 2015

Member

Ah OK I thought it worked in the parent class since you said this:

Using the $this->orderDomainService property works for the default Module, but not for the other Modules.

So it doesn't work in the parent class too if I understand correctly.

In that case this is indeed not normal as it should inject in all properties: we used the same pattern in another ZF1 project and I'm pretty sure we did something similar. I'm going to check a little more and come back to you.

Member

mnapoli commented May 26, 2015

Ah OK I thought it worked in the parent class since you said this:

Using the $this->orderDomainService property works for the default Module, but not for the other Modules.

So it doesn't work in the parent class too if I understand correctly.

In that case this is indeed not normal as it should inject in all properties: we used the same pattern in another ZF1 project and I'm pretty sure we did something similar. I'm going to check a little more and come back to you.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli May 26, 2015

Member

OK you are right this is a bug! We did use that pattern but we made the properties protected so that we could use then in the subclasses.

FYI there is a test for that: https://github.com/mnapoli/PHP-DI/blob/5.0/tests/IntegrationTest/InheritanceTest.php#L23-L53 but it tests for public properties, not private ones.

I will fix this in 5.0 thank you for reporting it!

Member

mnapoli commented May 26, 2015

OK you are right this is a bug! We did use that pattern but we made the properties protected so that we could use then in the subclasses.

FYI there is a test for that: https://github.com/mnapoli/PHP-DI/blob/5.0/tests/IntegrationTest/InheritanceTest.php#L23-L53 but it tests for public properties, not private ones.

I will fix this in 5.0 thank you for reporting it!

@mnapoli mnapoli added bug and removed support labels May 26, 2015

@mnapoli mnapoli added this to the 5.0 milestone May 26, 2015

@holtkamp

This comment has been minimized.

Show comment
Hide comment
@holtkamp

holtkamp May 26, 2015

Contributor

:), thanks for the swift responses!

Indeed, it works when using the default Module (which dispatches the
Default_OrderController). Only when using another Module, specialized
controllers are used, that is where the issue pops up. Glad you understood
me, great library BTW!
On May 26, 2015 3:08 PM, "Matthieu Napoli" notifications@github.com wrote:

Ah OK I thought it worked in the parent class since you said this:

Using the $this->orderDomainService property works for the default Module,
but not for the other Modules.

So it doesn't work in the parent class too if I understand correctly.

In that case this is indeed not normal as it should inject in all
properties: we used the same pattern in another ZF1 project and I'm pretty
sure we did something similar. I'm going to check a little more and come
back to you.


Reply to this email directly or view it on GitHub
#257 (comment).

Contributor

holtkamp commented May 26, 2015

:), thanks for the swift responses!

Indeed, it works when using the default Module (which dispatches the
Default_OrderController). Only when using another Module, specialized
controllers are used, that is where the issue pops up. Glad you understood
me, great library BTW!
On May 26, 2015 3:08 PM, "Matthieu Napoli" notifications@github.com wrote:

Ah OK I thought it worked in the parent class since you said this:

Using the $this->orderDomainService property works for the default Module,
but not for the other Modules.

So it doesn't work in the parent class too if I understand correctly.

In that case this is indeed not normal as it should inject in all
properties: we used the same pattern in another ZF1 project and I'm pretty
sure we did something similar. I'm going to check a little more and come
back to you.


Reply to this email directly or view it on GitHub
#257 (comment).

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jun 4, 2015

Member

Hi, FYI I will be rescheduling this issue to the next version. I'm sorry about that but v5.0 is ready to be released (I wanted to release this week) and when i started working on this issue I realized it requires a lot of work (at least more time than I have available right now).

In the meantime as a workaround you can define your properties as protected at injections will work.

Member

mnapoli commented Jun 4, 2015

Hi, FYI I will be rescheduling this issue to the next version. I'm sorry about that but v5.0 is ready to be released (I wanted to release this week) and when i started working on this issue I realized it requires a lot of work (at least more time than I have available right now).

In the meantime as a workaround you can define your properties as protected at injections will work.

@mnapoli mnapoli modified the milestones: 5.1, 5.0 Jun 4, 2015

@holtkamp

This comment has been minimized.

Show comment
Hide comment
@holtkamp

holtkamp Jun 4, 2015

Contributor

Ok, 100% no stress ;).

Not sure whether it is related, but I once had to follow a class hierarchy 'stream upwards' as well. In my case to reset all properties to their default values. Especially the private properties were not that trivial. Maybe you can use it as inspiration to inspect upstream defined class/object properties.

https://gist.github.com/holtkamp/f3a2ad32ca3866297c0f

Contributor

holtkamp commented Jun 4, 2015

Ok, 100% no stress ;).

Not sure whether it is related, but I once had to follow a class hierarchy 'stream upwards' as well. In my case to reset all properties to their default values. Especially the private properties were not that trivial. Maybe you can use it as inspiration to inspect upstream defined class/object properties.

https://gist.github.com/holtkamp/f3a2ad32ca3866297c0f

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jun 4, 2015

Member

👍 thanks

Member

mnapoli commented Jun 4, 2015

👍 thanks

@holtkamp

This comment has been minimized.

Show comment
Hide comment
@holtkamp

holtkamp Jul 8, 2015

Contributor

@mnapoli, need help / extra eyes with this or have you already pinned down the cause of the mentioned behaviour?

Contributor

holtkamp commented Jul 8, 2015

@mnapoli, need help / extra eyes with this or have you already pinned down the cause of the mentioned behaviour?

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 23, 2015

Member

@holtkamp sorry for the delay! I wrote a test reproducing it: https://github.com/PHP-DI/PHP-DI/compare/257-parent-private-properties But haven't started working on a fix yet :/

Member

mnapoli commented Jul 23, 2015

@holtkamp sorry for the delay! I wrote a test reproducing it: https://github.com/PHP-DI/PHP-DI/compare/257-parent-private-properties But haven't started working on a fix yet :/

@holtkamp

This comment has been minimized.

Show comment
Hide comment
@holtkamp

holtkamp Sep 8, 2015

Contributor

Great, just upgraded to https://github.com/PHP-DI/PHP-DI/tree/5.1.0 and everything works as expected when using private properties 👍

Contributor

holtkamp commented Sep 8, 2015

Great, just upgraded to https://github.com/PHP-DI/PHP-DI/tree/5.1.0 and everything works as expected when using private properties 👍

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