Skip to content

[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

Merged

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented May 7, 2020

Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? yes
Tickets -
License MIT
Doc PR -

I think the twig service don't need to be public anymore - we never need to access it directly in Symfony's code.

/**
* {@inheritdoc}
*/
public function build(ContainerBuilder $container)
Copy link
Contributor Author

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

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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');
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@fancyweb fancyweb force-pushed the twig-bundle-deprecate-public-twig-service branch from cf53d43 to 1490baa Compare June 9, 2020 09:47
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Can you rebase?

@fancyweb fancyweb force-pushed the twig-bundle-deprecate-public-twig-service branch from 1490baa to c278d09 Compare June 24, 2020 14:33
@fancyweb fancyweb force-pushed the twig-bundle-deprecate-public-twig-service branch from c278d09 to f64cbad Compare June 24, 2020 14:45
@fancyweb
Copy link
Contributor Author

@fabpot Done.

@fabpot
Copy link
Member

fabpot commented Jun 25, 2020

Thank you @fancyweb.

@fabpot fabpot merged commit ce7e39b into symfony:master Jun 25, 2020
@fancyweb fancyweb deleted the twig-bundle-deprecate-public-twig-service branch June 25, 2020 07:35
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants