Skip to content

[FrameworkBundle] Fix service reset between tests #58240

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

Merged
merged 1 commit into from
Sep 16, 2024
Merged

[FrameworkBundle] Fix service reset between tests #58240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Test/KernelTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@ protected static function ensureKernelShutdown()
static::$kernel->shutdown();
static::$booted = false;

if ($container->has('services_resetter')) {
// Instantiate the service because Container::reset() only resets services that have been used
$container->get('services_resetter');
}

if ($container instanceof ResetInterface) {
$container->reset();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$container->reset() must still be called, to reset the instances hold by the container itself (see the code of line 302).

Btw, if you ensure that the services_resetter service is instantiated, the container reset call will then call it (as ServiceResetter implements the ResetInterface).

The reason it was not called is because Container::reset only resets instantiated resettable services

Copy link
Contributor Author

@HypeMC HypeMC Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof Fixed, see https://github.com/symfony/symfony/compare/549ada5257c771da92d51aff76e224e1ac5d31f4..5491aa54a3205acfaf5ba62c134bddc26d794a36

Initially, I wanted to just instantiate the services_resetter and let Container::reset() call reset on it, but that didn’t work because the services_resetter only resets instantiated services. Because of line 302, by the time it’s called, there aren’t any instantiated services left.

The downside of the current approach is that some services will get reset twice, but I don’t see a better solution at the moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah indeed. The container resets itself before resetting the services.

I think this might actually be an issue if the reset method of a service triggers the instantiation of some other service. I think the resetting of the container state should probably be moved after the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof Done https://github.com/symfony/symfony/compare/5491aa54a3205acfaf5ba62c134bddc26d794a36..53d0f56be6a315055cb3ceef09a7f6bcb233a120

}
Expand Down
27 changes: 27 additions & 0 deletions ...eworkBundle/Tests/Functional/Bundle/TestBundle/TestServiceContainer/ResettableService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\TestServiceContainer;

class ResettableService
{
private $count = 0;

public function myCustomName(): void
{
++$this->count;
}

public function getCount(): int
{
return $this->count;
}
}
11 changes: 11 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Functional/KernelTestCaseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\TestServiceContainer\NonPublicService;
use Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\TestServiceContainer\PrivateService;
use Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\TestServiceContainer\PublicService;
use Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\TestServiceContainer\ResettableService;
use Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\TestServiceContainer\UnusedPrivateService;
use Symfony\Component\DependencyInjection\ContainerInterface;

Expand All @@ -41,4 +42,14 @@ public function testThatPrivateServicesAreAvailableIfTestConfigIsEnabled()
$this->assertTrue($container->has('private_service'));
$this->assertFalse($container->has(UnusedPrivateService::class));
}

public function testServicesAreResetOnEnsureKernelShutdown()
{
static::bootKernel(['test_case' => 'TestServiceContainer']);

$resettableService = static::getContainer()->get(ResettableService::class);

self::ensureKernelShutdown();
self::assertSame(1, $resettableService->getCount());
}
}
5 changes: 5 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Functional/app/TestServiceContainer/services.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,8 @@ services:
arguments:
- '@Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\TestServiceContainer\NonPublicService'
- '@Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\TestServiceContainer\PrivateService'

Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\TestServiceContainer\ResettableService:
public: true
tags:
- kernel.reset: { method: 'myCustomName' }
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/FrameworkBundle/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"ext-xml": "*",
"symfony/cache": "^5.2|^6.0",
"symfony/config": "^5.3|^6.0",
"symfony/dependency-injection": "^5.4.5|^6.0.5",
"symfony/dependency-injection": "^5.4.44|^6.0.5",
"symfony/deprecation-contracts": "^2.1|^3",
"symfony/event-dispatcher": "^5.1|^6.0",
"symfony/error-handler": "^4.4.1|^5.0.1|^6.0",
Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/DependencyInjection/Container.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,6 @@ public function initialized(string $id)
public function reset()
{
$services = $this->services + $this->privates;
$this->services = $this->factories = $this->privates = [];

foreach ($services as $service) {
try {
Expand All @@ -310,6 +309,8 @@ public function reset()
continue;
}
}

$this->services = $this->factories = $this->privates = [];
}

/**
Expand Down
Loading