8000 [FrameworkBundle] make resettable services configurable/opt-in on 3.4? · Issue #24552 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
dmaicher opened this issue Oct 13, 2017 · 6 comments
Closed

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

dmaicher opened this issue Oct 13, 2017 · 6 comments
Labels
FrameworkBundle

Comments

@dmaicher
Copy link
Contributor
dmaicher commented Oct 13, 2017 •
edited
Loading
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?
@dmaicher dmaicher changed the title [FrameworkBundle] make resettable services configurable on 3.4? [FrameworkBundle] make resettable services configurable/opt-in on 3.4? Oct 13, 2017
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Oct 13, 2017 •
edited
Loading

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?

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 13, 2017
@xabbuh
Copy link
Member
xabbuh commented Oct 13, 2017

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

@dmaicher dmaicher mentioned this issue Oct 13, 2017
Closed
4 tasks
@derrabus
Copy link
Member

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
Copy link
Contributor Author

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.

@nicolas-grekas nicolas-grekas removed this from the 3.4 milestone Oct 14, 2017
@derrabus
Copy link
Member
derrabus commented Oct 16, 2017 •
edited
Loading

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
Copy link
Contributor Author

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.

@nicolas-grekas nicolas-grekas mentioned this issue Oct 25, 2017
Closed
@derrabus derrabus mentioned this issue Oct 26, 2017
Closed
@nicolas-grekas nicolas-grekas mentioned this issue Oct 27, 2017
Merged
fabpot added a commit that referenced this issue Oct 30, 2017
@fabpot
bug #24709 [HttpKernel] Move services reset to Kernel::handle()+boot(…
039250a 
…) (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpKernel] Move services reset to Kernel::handle()+boot()

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24552
| License       | MIT
| Doc PR        | -

This is an alternative to #24697 (which uses middlewares).
This PR adds a new `services_resetter` service that the Kernel calls on 2nd root requests to reset services.
Instead of #24697 which plans for optional enabling of the services reset, this approach moves the responsibility of calling the services resetter to the core Kernel class, so that no configuration/middleware/etc. is required at all, and no overhead exists at all for regular requests.

Commits
-------

4501a36 [HttpKernel] Move services reset to Kernel
@fabpot fabpot closed this as completed Oct 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FrameworkBundle
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @javiereguiluz @nicolas-grekas @dmaicher @derrabus @xabbuh
0