Proxies for lazy loaded dependencies are not written to file? #411

Closed
holtkamp opened this Issue May 21, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@holtkamp
Contributor

holtkamp commented May 21, 2016

When:

  • configuring objects to be lazily loaded using the ObjectDefinitionHelper as suggested in the docs:
//definitions.php
array(
    Class1::class => object()->lazy(),
    Class2::class => object()->lazy(),
    ClassN::class => object()->lazy(),
)
  • storing the Definitions in a file and adding them like $builder->addDefinitions('definitions.php');
  • using the ocramius\proxy-manager
  • configuring the DI builder to write the proxies to a file: writeProxiesToFile(true, '/folderName')

Then:

  • no files are actually generated in the indicated folder
  • performance drops significantly.

When switching back to non-lazy configured objects, performance is back to normal again. After browsing around in the source code, it seems a FileWriterGeneratorStrategy should also be set explicitly, since it defaults to an EvaluatingGeneratorStrategy which removes the generated files afterwards

Adding one line to the ProxyFactory

if ($this->writeProxiesToFile) {
$config->setProxiesTargetDir($this->proxyDirectory);
spl_autoload_register($config->getProxyAutoloader());
resolves this:

$config->setGeneratorStrategy(new FileWriterGeneratorStrategy(new \ProxyManager\FileLocator\FileLocator($this->proxyDirectory)));

Also opened #412

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli May 22, 2016

Member

Oh wow thanks. This is not great indeed, but I think this is because of a BC break in ProxyManager 2.0 (https://github.com/Ocramius/ProxyManager/releases/tag/2.0.0): Ocramius/ProxyManager#105

The "officially recommended" version is ~1.0 (through docs and https://github.com/PHP-DI/PHP-DI/blob/master/composer.json#L44). suggests is showing its limits here. However it's of course great that we add support for 1.0 and 2.0, it was mentioned in #359 (comment) but I never got around doing it. If you could update your PR to keep the else { $config->setGeneratorStrategy(new EvaluatingGeneratorStrategy()); that way the factory would work with both versions?

If you have time you could also update composer.json and the docs, but if you don't that's not a problem I'll probably have some time later today for that.

Member

mnapoli commented May 22, 2016

Oh wow thanks. This is not great indeed, but I think this is because of a BC break in ProxyManager 2.0 (https://github.com/Ocramius/ProxyManager/releases/tag/2.0.0): Ocramius/ProxyManager#105

The "officially recommended" version is ~1.0 (through docs and https://github.com/PHP-DI/PHP-DI/blob/master/composer.json#L44). suggests is showing its limits here. However it's of course great that we add support for 1.0 and 2.0, it was mentioned in #359 (comment) but I never got around doing it. If you could update your PR to keep the else { $config->setGeneratorStrategy(new EvaluatingGeneratorStrategy()); that way the factory would work with both versions?

If you have time you could also update composer.json and the docs, but if you don't that's not a problem I'll probably have some time later today for that.

@mnapoli mnapoli added the bug label May 22, 2016

@holtkamp

This comment has been minimized.

Show comment
Hide comment
@holtkamp

holtkamp May 23, 2016

Contributor

If you could update your PR to keep the else { $config->setGeneratorStrategy(new EvaluatingGeneratorStrategy()); that way the factory would work with both versions?

Done!

If you have time you could also update composer.json and the docs, but if you don't that's not a problem I'll probably have some time later today for that.

Don't know whether that is still required, both versions are supported now right ? 😄

Contributor

holtkamp commented May 23, 2016

If you could update your PR to keep the else { $config->setGeneratorStrategy(new EvaluatingGeneratorStrategy()); that way the factory would work with both versions?

Done!

If you have time you could also update composer.json and the docs, but if you don't that's not a problem I'll probably have some time later today for that.

Don't know whether that is still required, both versions are supported now right ? 😄

@mnapoli mnapoli added this to the 5.3 milestone May 29, 2016

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli May 29, 2016

Member

Fixed in #412

Member

mnapoli commented May 29, 2016

Fixed in #412

@mnapoli mnapoli closed this May 29, 2016

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