Error compiling factory definitions since 6.0.7

[l-x]

<?php

declare(strict_types=1);

use function DI\autowire;

use DI\ContainerBuilder;
use function DI\factory;

require_once __DIR__.'/../vendor/autoload.php';

class Fu
{
    public function create(): self
    {
        return new static();
    }
}

class Bar
{
    public function create(): self
    {
        return new static();
    }
}

class FuBar
{
    private $fu;

    private $bar;

    public function __construct(Fu $fu, Bar $bar)
    {
        $this->fu = $fu;
        $this->bar = $bar;
    }
}

$builder = new ContainerBuilder();
$builder->enableCompilation(__DIR__);

$builder->addDefinitions([
    FuBar::class => autowire()
        ->constructorParameter('fu', factory([Fu::class, 'create']))
        ->constructorParameter('bar', factory([Bar::class, 'create']))
]);

$dic = $builder->build();

In CompiledContainer.php you'll find

    protected function get3b62a97e28ef909b96b42a793be8a616()
    {
        $object = new FuBar($this->get5ffee756f39d463eb2a0c22906c0f2be(), $this->get5ffee756f39d463eb2a0c22906c0f2be());
        return $object;
    }

As you can see the definition for Bar (Method get5ffee756f39d463eb2a0c22906c0f2be) is used for every Parameter in FuBar::__construct.

I was able to nail the issue down to \DI\Compiler\Compiler::getHashedValue. In this scenario the parameter string $value is <nested definition>Factory for every factory definition used like above.

In our case this is a crititcal issue, downgrading to 6.0.6 fixed this.

Relates to #605

[l-x]

Just cleaned up the example code

[mnapoli]

Thanks for the report!

@holtkamp could it be related to the discussion in here? But it was about closures 🤔 so I'm not so sure, it's been a long time. If you have any insight!

[holtkamp]

mm, yeah, apparently this way of using the factory() is not covered yet by the tests:

https://github.com/PHP-DI/PHP-DI/blob/78326779037852fb8c13be606110434b9911eebd/tests/IntegrationTest/CompiledContainerTest.php

Let's try to reproduce it in a test and then see what the options are...

[holtkamp]

Reproduced it here: https://github.com/holtkamp/PHP-DI/blob/patch-648/tests/IntegrationTest/CompiledContainerTest.php#L89-L91

[holtkamp]

@mnapoli guess you were right here: #605 (comment)

I included the "fingerprint" of the callable in FactoryDefinition::__toString():

https://github.com/holtkamp/PHP-DI/blob/9fd45ce9a00b1eb277456e371a454686b50a6f41/src/Definition/FactoryDefinition.php#L75-L79

And appended a hash of the closure code:

https://github.com/holtkamp/PHP-DI/blob/9fd45ce9a00b1eb277456e371a454686b50a6f41/src/Compiler/Compiler.php#L265

This make the tests pass, but probably more "advanced" examples can be added to the test set to reduce the risk of missing more cases?

@mnapoli you can see the changes here: master...holtkamp:patch-648

@l-x can you check whether patch-branch https://github.com/holtkamp/PHP-DI/tree/patch-648 resolves the problems you had?

[l-x]

First thanks alot for your extraordinary quick response! :)

@holtkamp Your patch solves the initial problem but introduces a new one:

<?php

declare(strict_types=1);

use DI\ContainerBuilder;

require_once __DIR__.'/../vendor/autoload.php';

$builder = new ContainerBuilder();
$builder->enableCompilation(__DIR__);

$builder->addDefinitions([
    'herp' => function () { return 'derp'; }
]);

$dic = $builder->build();

If you have a look at the const METHOD_MAPPING in the compiled container you'll see that the change of the generated method name is not done in this array. But I guess this should be not a big thing :)

[holtkamp]

@l-x thanks for diving into this again. Pushed some more changes to https://github.com/holtkamp/PHP-DI/tree/patch-648

@mnapoli this approach does not win a price of "most elegant" code, so we might have to think of a better way to "identify" such closures

[mnapoli]

Thanks @holtkamp for jumping on this! I'll have a look at the PR (I was away this weekend)

[holtkamp]

@mnapoli I pushed some changes in #649 (comment), can you have a look?

[mnapoli]

@holtkamp 👍 trying some things locally at the moment.

[mnapoli]

Following the discussion in #649 I have opened #652 based on #649.

@l-x could you give it a try to see if it fixes your problem?

[mnapoli]

@l-x did you have a chance to try #652?

@holtkamp maybe you could give it a go as well to see if there are any regression for you?

[holtkamp]

@mnapoli I gave it a shot with the https://github.com/PHP-DI/PHP-DI/tree/fix-648 branch by using dev-fix-648 in my composer.json file.

Compilation for GuzzleHttp\Handler\CurlMultiHandler triggers a fatal error in the generated proxy:

Fatal error: Uncaught Error: Call to a member function __destruct() on null in /project/data/dependencyInjectionContainerProxies/ProxyManagerGeneratedProxy__PM__GuzzleHttpHandlerCurlMultiHandlerGenerated0ad1b0321b82bd00ded596ecfd75f94a.php:31 Stack trace: 
#0 /project/vendor/php-di/php-di/src/Proxy/ProxyFactory.php(60): ProxyManagerGeneratedProxy\__PM__\GuzzleHttp\Handler\CurlMultiHandler\Generated0ad1b0321b82bd00ded596ecfd75f94a->__destruct() 
#1 /project/vendor/php-di/php-di/src/Proxy/ProxyFactory.php(74): DI\Proxy\ProxyFactory->createProxy('GuzzleHttp\\Hand...', Object(Closure))
#2 /project/vendor/php-di/php-di/src/Compiler/ObjectCreationCompiler.php(142): DI\Proxy\ProxyFactory->generateProxyClass('GuzzleHttp\\Hand...')
#3 /project/vendor/php-di/php-di/src/Compiler/ObjectCreationCompiler.php(39) 

The PHP-DI definition is quite simple:

\GuzzleHttp\Handler\CurlMultiHandler::class => \DI\autowire()->lazy(),

The relevant part of the generated PHP code is:

namespace ProxyManagerGeneratedProxy\__PM__\GuzzleHttp\Handler\CurlMultiHandler;

class Generated0ad1b0321b82bd00ded596ecfd75f94a extends \GuzzleHttp\Handler\CurlMultiHandler implements \ProxyManager\Proxy\VirtualProxyInterface
{
    /**
     * @var \Closure|null initializer responsible for generating the wrapped object
     */
    private $valueHoldere7a73 = null;

    /* other properties and functions */

    public function __destruct()
    {
        $this->initializer6398d && $this->initializer6398d->__invoke($this->valueHoldere7a73, $this, '__destruct', array(), $this->initializer6398d);

        return $this->valueHoldere7a73->__destruct();
    }

The property valueHoldere7a73 is not initialized when __destruct() is invoked on it.

I am using:

• PHP 7.3.3
• guzzlehttp/guzzle 6.3.3
• ocramius/proxy-manager 2.2.2
  • this seems the class that generates this property. https://github.com/Ocramius/ProxyManager/blob/2.2.2/src/ProxyManager/ProxyGenerator/LazyLoadingValueHolder/PropertyGenerator/ValueHolderProperty.php

Not sure whether this is a PHP-DI issue, or a ProxyManager issue?

[mnapoli]

Oh wait I think this isn't related to #652 but because of this change: https://github.com/PHP-DI/PHP-DI/pull/645/files#diff-df30424a53a66e12594a2c5cdbe534a1R74

The idea was to "get" the proxy to force writing it to disk. But by doing that we initialize those proxies in a weird way, and the __destruct() doesn't seem to like it :/

[holtkamp]

ah, yeah, you are right.

Switched back to 6.0.7 and the same fatal error is generated.

Currently I do not use the compiled container in big projects because of the (perceived) performance degradation as described here: #615

So I guess this is a regression introduced earlier 🤓

[l-x]

Finally I am back again ^^

Is there a patch I can test my project against?

[holtkamp]

@l-x I suppose you can try the https://github.com/PHP-DI/PHP-DI/tree/fix-648 branch by using dev-fix-648 in your composer.json file...

[l-x]

I had a chance to try the fix, unfortunately a fatal error occurs:

PHP Fatal error:  Uncaught DI\Definition\Exception\InvalidDefinition: Entry "JsonRpc\Server\Server" cannot be compiled: Internal PHP-DI error (please report this): Entry "" cannot be compiled. A method with the same name (getbeb0762a5b744b647ce378616591c05b) already exists pointing to code `return $this->resolveFactory([
            0 => 'Psr\\Log\\LoggerInterface',
            1 => 'withName',
        ], '', [
            'name' => 'API',
        ]);`, while this one points to code `return $this->resolveFactory([
            0 => 'Psr\\Log\\LoggerInterface',
            1 => 'withName',
        ], '', [
            'name' => 'JsonRpc',
        ]);`.

[hultberg]

Any progress on getting this issue fixed? The v6.0.7 release has been out for almost a month with this critical issue.

[mnapoli]

@hultberg you are welcome to help, on my side I am doing my best with the time I can find here and there.

Alternatively you can support me: send me a mail (my email is on my public profile) and I can bill you (or your company) to set aside some client work and spend a day or two to work on this. That should definitely help move things faster.

[mnapoli]

I think I've hit a big obstacle regarding the approach in #652.

What #652 does is generate a unique hash per definition (the key of v6.0.7 is that such hash must be constant). The hash is based on the definition itself, and that's why #652 is such a huge refactoring.

But while trying to fix #648 (comment) I noticed that sometimes the name of the definition is actually used in the code generated itself. That makes it even harder to do what was initially planned.

At this point I think the safest option is to revert back to v6.0.6 (revert #605) until we find a better solution.

Is there any other way to solve the original problem with Heroku?

Feel free to share what you think here, unless there's any opposition to this I should be able to do that tomorrow.

[holtkamp]

Indeed, maybe we should conclude that this whole idea for idempotent containers was a bridge too far 😨

For the time being we could:

• prevent complicating the code base too much
• say goodbye to the idea of idempotent compiled container
• document the possible problems when using "free" Heroku dynos which go to sleep after 15 minutes., after all this is for hobby/development purposes (in which compiling the container might not even make a lot of sense?)

This way a stable 6.0.x can be released.

@mnapoli thanks for the effort you gave put into this because of my initial request, maybe someday we can tackle this challenge in the future 😄

[mnapoli]

👍 thanks for your efforts as well! Too bad this doesn't pays off as expected. I've planned to work on this tomorrow evening.

[mnapoli]

@l-x @hultberg I've reverted the changes of 6.0.7 on master.

Could you try dev-master in composer.json and confirm that everything is alright? I'd like to be sure before tagging the release ;)

Thanks!

[mnapoli]

ping @l-x @hultberg could you confirm please?

[mnapoli]

I've tagged a new release with the fixes.

@hultberg thank you for your support…

[hultberg]

@mnapoli I can confirm that this issue has been fixed with v6.0.8.