[FrameworkBundle] make resettable services configurable/opt-in on 3.4?

[dmaicher]

Q	A
Bug report?	no
Feature request?	yes
BC Break report?	yes
RFC?	yes
Symfony version	3.4

I found some issues when testing Symfony 3.4.x-dev on one of my biggest apps.

With this PR #24155 the resettable services have been introduced and it seems this is quite a behavior change for example within functional phpunit tests.

In our tests we have some cases where we use a Symfony\Bundle\FrameworkBundle\Client and perform some requests on our app. Afterwards we access the container of the client's kernel to retrieve some services that are now suddenly reset after the request 😕

I'm wondering if this new resettable stuff should be opt-in? I mean apart from my (probably not so hard to fix) test issues this will also consume CPU cycles on production where we have a normal php-fpm "shared nothing" setup like probably most people out there?

For now I would probably add this to my app:

class RemoveServiceResetListenerPass implements CompilerPassInterface
{
    public function process(ContainerBuilder $container)
    {
        if ($container->hasDefinition(ServiceResetListener::class)) {
            $container->removeDefinition(ServiceResetListener::class);
        }
    }
}

What do you think?

[nicolas-grekas]

I think the CPU argument is negligible, the reset methods do nothing. But for the "functional tests" one, I have no contra-argument. We need to support your use case as it was supported before.
So, we have to add an option in the config, isn't it? Do we have any other choice?

[xabbuh]

Why not. But that option should probably be opt-in for BC.

[derrabus]

I'm curious: What kind of functional test is broken by the reset feature? And would it help to perform your tests before terminating the kernel?

[dmaicher]

One example is a functional login test that asserts some permissions of the user after performing the request:

public function testAdminLogin()
{
    $this->submitLoginForm($this->user->getEmail(), TestFixtures::USER_PLAINTEXT_PASSWORD);
    $this->assertTrue($this->client->getContainer()->get('security.authorization_checker')->isGranted(Roles::ROLE_ADMIN));
}

Leads now to:

Symfony\Component\Security\Core\Exception\AuthenticationCredentialsNotFoundException: The token storage contains no authentication token. One possible reason may be that there is no firewall configured for this URL.

One other test uses a client to perform a request and then uses some services like this:

public function setUp()
{
    parent::setUp();
    $client = $this->getAuthorizedClient();
    $client->request('GET', '/');
    $this->httpKernel = $client->getContainer()->get('http_kernel');
    $this->listener = $client->getContainer()->get('ca_core.kernel_exception_listener');
    $this->tokenStorage = $client->getContainer()->get('security.token_storage'); //now empty after reset
}

There are some other tests that fail with a weird Error: Class 'appTestProjectContainer' not found. Did not debug those further.

I'm not saying this is impossible to fix 😉 There are probably nicer ways of testing things. But this seems like a behavioral change to me.

[derrabus]

The problem is that you're asserting request-related state on a terminated kernel. To me, it feels like this has worked "by accident" so far, but I understand that the reset feature will break quite some tests in many Symfony projects out there. Also, since the BrowserKit client terminates the kernel immediately, you don't have a chance to query that kind of information before termination.

I see why you would want to add that switch to solve your problem. And I could live with such a switch – in fact I wanted to introduce that switch myself in the first place. Anyhow, I feel like the introduction happens for all the wrong reasons here:

• Approaches like PHPPM and functional testing should not exclude each other. But this would be the consequence, if we say you can only reasonably use WebTestCase if you turn off resetting.
• WebTestCase to me looks like a promising candidate of receiving a speedup through the kernel reset instead of booting a new container for each test. So I'd rather want to make use of the reset feature in functional tests (maybe in a later Symfony release) than to cut off those test from it completely.

Any thoughts?

[dmaicher]

I think when actually using this feature on production it will be fine that it "breaks" writing tests like the ones I mentioned above. So it should then indeed be possible to also use this feature in the WebTestCase to not always reboot the kernel but to simply "recycle" it for a configured number of times? So the tests are actually closer to the production setup.