-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Always preload services classes tagged with container.preload #36742
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.
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
Conversation
8e51723
to
6a65742
Compare
if ('\\' === \DIRECTORY_SEPARATOR) { | ||
$dump = str_replace('\\\\Fixtures\\\\includes\\\\foo.php', '/Fixtures/includes/foo.php', $dump); | ||
} | ||
$this->assertStringMatchesFormatFile(self::$fixturesPath.'/php/services9_as_files.txt', $dump); | ||
$this->assertStringEqualsFile(self::$fixturesPath.'/php/services9_as_files.txt', $dump); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use a regex anymore because the file to compare to have become too big (preg_match(): Compilation failed: regular expression is too large at offset 35752
). I guess this strategy is useful when merging from lower branches? Do someone have a better idea than strict comparison or creating a new file? 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing: I need to recheck the strategy.
@@ -193,4 +193,13 @@ | |||
->addTag('container.preload', ['class' => 'Some\Sidekick1']) | |||
->addTag('container.preload', ['class' => 'Some\Sidekick2']); | |||
|
|||
$container->register('no_preload', 'TestNoPreload') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also added a test with container.no_preload
as I think it was not tested.
I feel like we might not need this PR after #36749 At least the case we built this solution for ( Agreed? |
I agree, it's better. |
This PR aims to solve the case where you want to force the preload of a service class even if the service definition has the
container.no_preload
tag.For example, in
RegisterListenersPass
, we add thecontainer.no_preload
tag on definitions that listen to some particular events. What if I want to force the preload of those services classes anyway?Adding the
container.preload
tag does not work because the "no preload" behavior has the priority. I propose to change that so one can simply add the tag when he wants to override Symfony's default behavior.Moreover, to not repeat the service class on the
container.preload
class
attribute, let's make this attribute optional. When it's not there, it means I want to preload the service class.