Container::__clone is missing

[chx]

When writing a unit test for Drupal, I found the parameter bag was not cloned so at least

public function __clone() {
    $this->parameterBag = clone $this->parameterBag;
}

but that's nowhere near enough as services , scopes etc needs to be cloned too.

[fabpot]

Indeed, a Container is not meant to be cloned. Not sure if we want to support this use case. Can you tell us a bit more about why you want to clone a Container?

[chx]

This test uses it. Yes, it's broken, it's being refactored at https://www.drupal.org/node/2531408

[stof]

IMO, we should rather forbid cloning the container rather than trying to support cloning of containers, which will be a huge pain (and even more if we want to support cloning ContainerBuilder as we would have to clone everything in it, but preserving circular references between cloned Definition objects, which is a huge pain)

[chx]

I agree with @stof : throwing an exception forbidding such cloning is a good solution.

[lastzero]

I'm actually cloning the container because completely building the container from scratch would be way too slow (https://github.com/lastzero/test-tools):

"Since global state must be avoided while performing tests, the service instances are not cached between tests. The service definitions in the container are reused however. This significantly improves test performance compared to a full container reinitialization before each test (about 5 to 10 times faster)."

I'm apparently not having any trouble with this so far, even though @stof states that container cloning is a "huge pain". Maybe that's just the case under certain conditions?

<?php

namespace TestTools\TestCase;

use Symfony\Component\DependencyInjection\ContainerBuilder;

/**
 * @author Michael Mayer <michael@lastzero.net>
 * @package TestTools
 * @license MIT
 */
class TestContainerBuilder extends ContainerBuilder
{
    public function clearInstances()
    {
        $this->services = array();
    }

    public function __clone()
    {
        $this->clearInstances();
    }
}

[stof]

@lastzero when cloning your ContainerBuilder, all definitions are still shared between both containers, and they are mutable. This means that mutating a container is also mutating others for more than half of the possible mutations. This is not a good way to clone the container.

What you want in your case is probably to reset the container (which is available in Symfony 2.8) and keep using the same instance.

[lastzero]

When cloning the container, I'm intentionally reusing the service definitions exactly as they were (with the exception of %fixture.path%, which is different for each test case - but that works just fine). I just need to get rid of the service instances between tests, which is what Container::reset() does now (thanks a lot for the hint!). Of course, if you change the definitions, it totally makes to build a new container and not reuse an existing one. As I understand you, calling reset() should be enough and cloning is not required. I probably thought that's more secure to avoid possible side effects of reusing the same container instance twice (test isolation is very important).

[stof]

@lastzero replacing a parameter may not work fine if it has been inlined in service definitions already.
And the fact that you clone the container partially makes it less secure regarding isolation IMO, given that you make the user think it is a different one, while many thing are shared

[lastzero]

@stof Thanks again. It's awesome to get quick feedback like this, but I don't want to steal too much of your time. Let me figure out a way to simulate the behaviour you describe with a unit test, so that I can get a better understanding of the potential issue (which I didn't observe in practice so far).

[lastzero]

@stof Calling reset() instead of cloning works perfectly (thanks for adding this method). However, my tests didn't reveal any parameter inlining or caching. As this test demonstrates, the container always uses the current parameters for creating new instances: https://github.com/lastzero/test-tools/blob/master/src/TestTools/Tests/TestCase/UnitTestCaseTest.php

[stof]

@lastzero this is because you don't call compiler passes in your ContainerBuilder, so the inlining does not happen. but other action may not happen either (for instance, if you use service decoration, it won't be taken into account as this is done by a compiler pass)

[lastzero]

@stof Thanks for confirming, that's perfect, as long as you don't change it. Works as designed. Using the container for unit testing is already a bold move, so I certainly don't want to support obfuscating features like service decoration for this - especially if it has a negative impact on performance.

[stof]

@lastzero service decoration is not an obfuscating feature. It is an extendability feature.
And many services are built thanks to compiler passes too, not only decorated one. Any time you use a tag on a service, it means a compiler pass will do some work related to this service (generally injecting it into another one).
Compiling a container before using it at runtime is actually not optional.

[lastzero]

@stof Rebuilding the container before each test would slow down test execution by a factor of ten without delivering any enough value. Less is more in this case. I agree that compilation and caching are not optional when using the container in an application context!

[lastzero]

@stof Actually, if you manage to allow recompilation without having to load the config from files, that'd be awesome. But right now, I'm happy how well it works for my very specific use case. It's still an improvement over creating objects manually, especially since you can reuse the existing test container config for your bundle or application (the other way doesn't always work, as you explained).

[stof]

@lastzero the compilation step does half of the work of building the container (maybe even more than that depending on the cases). We cannot allow redoing the second part of the work (it would require saving the state of the ContainerBuilder before that, which would be a nightmare as all the object graph is mutable)