-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[TwigBundle] Deprecate the public "twig" service to private #36739
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
[TwigBundle] Deprecate the public "twig" service to private #36739
Conversation
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function build(ContainerBuilder $container) |
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.
Since this test bundle is used with different configurations, it's easier to register those controllers as services here (it avoids duplication in config files).
|
||
public function __construct(ContainerInterface $container) | ||
{ | ||
$this->container = $container; |
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.
why not inject dependencies explicitly?
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 tried to modify existing tests the least possible. I mean I tried to keep $container->get
. I guess we can inject the dependencies as well.
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.
Using the service subscriber system looks OK to me 🤷♂️
@@ -40,7 +40,7 @@ public function testBundlePublicDir() | |||
public function testBundleTwigTemplatesDir() | |||
{ | |||
static::bootKernel(['test_case' => 'BundlePaths']); | |||
$twig = static::$container->get('twig'); | |||
$twig = static::$container->get('twig.alias'); |
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.
can't we get private services in tests with the special test container? why is this needed?
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.
The twig
service is still public. We don't want to access to it directly to avoid the deprecation.
cf53d43
to
1490baa
Compare
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.
Can you rebase?
1490baa
to
c278d09
Compare
c278d09
to
f64cbad
Compare
@fabpot Done. |
Thank you @fancyweb. |
I think the
twig
service don't need to be public anymore - we never need to access it directly in Symfony's code.