Document limitations when using lazy loaded dependencies that involve variadic functions #359

Closed
holtkamp opened this Issue Dec 30, 2015 · 19 comments

Comments

Projects
None yet
3 participants
@holtkamp
Contributor

holtkamp commented Dec 30, 2015

When using lazy loaded dependencies, I encountered some curious behaviour when handing over more parameters to a function than defined in its signature (variadic functions).

class Service
{
    /**
     * @param mixed $param1
     * @param mixed $param2
     * @param mixed $paramN Can be used to supply additional parameters
     */
    public function doStuf($param1, $param2){
        echo func_num_args();
    }
}

Now let's try to use this Service when fetched from the dependency injection container

$dic->get('Service')->doStuff(1, 2, 3); //Prints '3' when not lazy loaded
$dic->get('Service')->doStuff(1, 2, 3); //Prints '2' when lazy loaded

I use this approach to inject extra variables when translating messages like: "User X has logged in on Y", where X and Y would be 'additional' parameters.

This probably has to do with the intermediate proxy classes that are generated. For example from a generated proxy class:

  /**
     * {@inheritDoc}
     */
    public function doStuf($param1, $param2)
    {
        $this->initializer5683c4a86687a054344119 && $this->initializer5683c4a86687a054344119->__invoke($this->valueHolder5683c4a866860700867865, $this, 'fireEventOnOrder', array('param1' => $param1, 'param2' => $param2), $this->initializer5683c4a86687a054344119);

        return $this->valueHolder5683c4a866860700867865->fireEventOnOrder($param1, $param2);
    }

When looking at Ocramius/ProxyManager#177, I think this behaviour is expected. Therefore I think it should be mentioned in the documentation as a warning...

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Dec 30, 2015

Member

Well thank you for going so far in the debugging :)

Ping @Ocramius. It seems Ocramius/ProxyManager#177 is about supporting PHP 5.6's variadics (the ... syntax), so I'm guessing it might simply be a bug in ProxyManager? (i.e. it should use func_get_args() instead of listing each parameter?)

Member

mnapoli commented Dec 30, 2015

Well thank you for going so far in the debugging :)

Ping @Ocramius. It seems Ocramius/ProxyManager#177 is about supporting PHP 5.6's variadics (the ... syntax), so I'm guessing it might simply be a bug in ProxyManager? (i.e. it should use func_get_args() instead of listing each parameter?)

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Dec 30, 2015

@mnapoli I still need to pack variadics, scalar type hints and return type hints into ProxyManager.

@mnapoli I still need to pack variadics, scalar type hints and return type hints into ProxyManager.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Dec 30, 2015

Member

Just to be sure, when you say variadics you also mean "PHP < 5.6 kind of variadics" which consists of passing more parameters than defined right?

In that case indeed we could document in PHP-DI that this is not supported right now to avoid confusion.

Member

mnapoli commented Dec 30, 2015

Just to be sure, when you say variadics you also mean "PHP < 5.6 kind of variadics" which consists of passing more parameters than defined right?

In that case indeed we could document in PHP-DI that this is not supported right now to avoid confusion.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Dec 30, 2015

Just to be sure, when you say variadics you also mean "PHP < 5.6 kind of variadics" which consists of passing more parameters than defined right?

No, I mean 5.6 variadics (... operator).

Pre-PHP-5.6 "variadic arguments" are totally ignored. Going to open an issue for that now.

Just to be sure, when you say variadics you also mean "PHP < 5.6 kind of variadics" which consists of passing more parameters than defined right?

No, I mean 5.6 variadics (... operator).

Pre-PHP-5.6 "variadic arguments" are totally ignored. Going to open an issue for that now.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Dec 30, 2015

Note: this is also broken in doctrine2, now that I notice it (known, but undocumented limitation).

Actually, the lazy loading ghost objects in ProxyManager 2.x work with that one, but I still need to implement them.

Opened Ocramius/ProxyManager#265 to track that, thanks!

Note: this is also broken in doctrine2, now that I notice it (known, but undocumented limitation).

Actually, the lazy loading ghost objects in ProxyManager 2.x work with that one, but I still need to implement them.

Opened Ocramius/ProxyManager#265 to track that, thanks!

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Dec 30, 2015

Member

Thanks!

Member

mnapoli commented Dec 30, 2015

Thanks!

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Dec 30, 2015

I provided documentation for this sort of problem in ProxyManager and fixed the support of variadic arguments in the 2.x branch.

There is also a possible resolution path for this particular issue at Ocramius/ProxyManager#269, but it is complex and for a super-edge case.

I'll get a 2.0 release out soon, but be aware that:

  • it will work with PHP "~5.6|~7.0"
  • there will be some BC breaks

I provided documentation for this sort of problem in ProxyManager and fixed the support of variadic arguments in the 2.x branch.

There is also a possible resolution path for this particular issue at Ocramius/ProxyManager#269, but it is complex and for a super-edge case.

I'll get a 2.0 release out soon, but be aware that:

  • it will work with PHP "~5.6|~7.0"
  • there will be some BC breaks
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Dec 30, 2015

Member

👍 I was just reading this ;) Thanks

Member

mnapoli commented Dec 30, 2015

👍 I was just reading this ;) Thanks

@holtkamp

This comment has been minimized.

Show comment
Hide comment
@holtkamp

holtkamp Feb 27, 2016

Contributor

just checking on the status on this to see whether this issue can be closed...

ProxyManager 2.0 have been released for a while now and Ocramius/ProxyManager#265 is closed. But I am not sure whether the named 'Ghost objects' which should support the pre-PHP-5.6 variadic functions is already implemented.

Contributor

holtkamp commented Feb 27, 2016

just checking on the status on this to see whether this issue can be closed...

ProxyManager 2.0 have been released for a while now and Ocramius/ProxyManager#265 is closed. But I am not sure whether the named 'Ghost objects' which should support the pre-PHP-5.6 variadic functions is already implemented.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Feb 29, 2016

Member

But it seems that v2 requires PHP 7 right?

Member

mnapoli commented Feb 29, 2016

But it seems that v2 requires PHP 7 right?

@holtkamp

This comment has been minimized.

Show comment
Hide comment
@holtkamp

holtkamp Feb 29, 2016

Contributor

aah, yeah seems to be the case: https://github.com/Ocramius/ProxyManager/blob/6c89b7bd6039d8047b1473e2074cb56baa4bc15d/composer.json#L22

Can we then conclude this will not be resolved for the time being and this limitation needs to be documented?

Contributor

holtkamp commented Feb 29, 2016

aah, yeah seems to be the case: https://github.com/Ocramius/ProxyManager/blob/6c89b7bd6039d8047b1473e2074cb56baa4bc15d/composer.json#L22

Can we then conclude this will not be resolved for the time being and this limitation needs to be documented?

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Feb 29, 2016

Member

Yep!

Member

mnapoli commented Feb 29, 2016

Yep!

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Mar 4, 2016

Note: you can require ~1.0|~2.0, since the API didn't change massively. Please see symfony/symfony#17919 for a reference implementation of a BC compliant way of doing this.

As for the rest: you simply need to document that lazy services with variadic signatures require PHP 7 and the latest ProxyManager.

Ocramius commented Mar 4, 2016

Note: you can require ~1.0|~2.0, since the API didn't change massively. Please see symfony/symfony#17919 for a reference implementation of a BC compliant way of doing this.

As for the rest: you simply need to document that lazy services with variadic signatures require PHP 7 and the latest ProxyManager.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Mar 9, 2016

Member

👍 good idea! Thanks for the links

Member

mnapoli commented Mar 9, 2016

👍 good idea! Thanks for the links

@holtkamp

This comment has been minimized.

Show comment
Hide comment
@holtkamp

holtkamp Mar 9, 2016

Contributor

Aah, nice approach 😉

Contributor

holtkamp commented Mar 9, 2016

Aah, nice approach 😉

@mnapoli mnapoli added the easy-pick label Mar 15, 2016

@holtkamp

This comment has been minimized.

Show comment
Hide comment
@holtkamp

holtkamp Mar 29, 2016

Contributor

@mnapoli are you considering adopting the suggestion of @Ocramius ? Guess it would be similar to: https://github.com/doctrine/migrations/blob/5f9429e5e6dd31d0f36c8951daa184112a39e195/composer.json#L18

Contributor

holtkamp commented Mar 29, 2016

@mnapoli are you considering adopting the suggestion of @Ocramius ? Guess it would be similar to: https://github.com/doctrine/migrations/blob/5f9429e5e6dd31d0f36c8951daa184112a39e195/composer.json#L18

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Mar 29, 2016

Member

Oh yes I'm definitely up for it, I just didn't have time to implement it yet. I guess what's to do:

  • update ProxyFactory if necessary to make it compatible with both version (since it's very simple it's possible there is nothing to do)
  • require ~1.0|~2.0, tests should run with highest and lowest version on Travis automatically
  • update the documentation

If you are up for it I'll be 👍 to merge it!

Member

mnapoli commented Mar 29, 2016

Oh yes I'm definitely up for it, I just didn't have time to implement it yet. I guess what's to do:

  • update ProxyFactory if necessary to make it compatible with both version (since it's very simple it's possible there is nothing to do)
  • require ~1.0|~2.0, tests should run with highest and lowest version on Travis automatically
  • update the documentation

If you are up for it I'll be 👍 to merge it!

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli May 29, 2016

Member

Fixed in #414 (PHP-DI can now be installed with ProxyManager ~1.0 or ~2.0).

Member

mnapoli commented May 29, 2016

Fixed in #414 (PHP-DI can now be installed with ProxyManager ~1.0 or ~2.0).

@mnapoli mnapoli closed this May 29, 2016

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius May 29, 2016

@mnapoli awesome! I really hope to push those 2.0 installations up (too many 1.x users ;-) )

@mnapoli awesome! I really hope to push those 2.0 installations up (too many 1.x users ;-) )

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