Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
added Twig runtimes for "critical" Twig extensions#20094
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| * | ||
| * @see FragmentHandler::render() | ||
| */ | ||
| public function renderFragment($uri, $options = array()) |
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.
While I agree with this change, it might break hacky solutions that wrap around this using decorators or extending this extension.
Twig extensions are not explicitly mentioned in the BC promises but should imo be considered internal (thus would allow this change)
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.
Let's be pragmatic here. This is definitely not an extension point.
| * | ||
| * @author Fabien Potencier <fabien@symfony.com> | ||
| */ | ||
| class HttpKernelRuntime |
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.
As usual: any reason to allow inheritance?
| { | ||
| return $this->handler->render($uri, $strategy, $options); | ||
| } | ||
| public function controller($controller, $attributes = array(), $query = array()) |
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.
if we want to optimize even further, we could turn such methods into static methods
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.
done
| @@ -71,5 +71,17 @@ public function process(ContainerBuilder $container) | |||
| if ($container->has('assets.packages')) { | |||
| $container->getDefinition('twig.extension.assets')->addTag('twig.extension'); | |||
| } | |||
| if (class_exists('Symfony\Component\Yaml\Parser')) { | |||
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.
As the definition of the container now depends on the existence of a class, we should define aClassExistenceResource and add such a resource to the container for such patterns (outside the if), so that the debug container is reloaded if you install the class
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.
That's for another PR as this is not the only place where we are doing this in a compiler pass.
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.
see#20121
…onent is not available
…ssionLanguage component is not available
…bpot)This PR was merged into the 3.2-dev branch.Discussion----------added Twig runtimes for "critical" Twig extensions| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aThis PR converts some Twig extensions to use a separate runtime. It also contains some optimizations to not load extensions when the associated component is not installed.Commits-------c541804 fixed tests812fbb4 decoupled the Twig HttpKernelExtension runtime from the extension79efb4c [TwigBundle] removed ExpressionLanguage Twig extension when the ExpressionLanguage component is not available3d4ad0b [TwigBundle] removed Stopwatch Twig extension when the Stopwatch component is not availabled0792e4 [TwigBundle] removed YAML Twig extension when the YAML component is not available
This PR was merged into the 3.2-dev branch.Discussion----------Class existence resource| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | see#20094| License | MIT| Doc PR | n/aCommits-------222b56d [TwigBundle] added support for ClassExistenceResource when relevantd98eb7b [Config] added ClassExistenceResource
This PR converts some Twig extensions to use a separate runtime. It also contains some optimizations to not load extensions when the associated component is not installed.