Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
added a Twig runtime loader#20092
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
6c63f41 todee1e09Comparedee1e09 to76260c9Compare| publicfunction__construct(ContainerInterface$container,array$mapping) | ||
| { | ||
| $this->mapping =$mapping; | ||
| } |
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.
Injecting a container but not actually assigning it
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.
fixed, added some tests as well :)
f701d66 to447d443Comparelinaori commentedSep 29, 2016
Seems like the TwigBundle and SecurityBundle are having some difficulties on 5.5, I think their constraints are not solid for 5.5 |
447d443 to6a8ed85Compare6a8ed85 to7fc174bComparefabpot commentedSep 29, 2016
It was just because Twig 1.x was not merged yet into master, fixed now. |
This PR was merged into the 3.2-dev branch.Discussion----------added a Twig runtime loader| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | seetwigphp/Twig#1913| License | MIT| Doc PR | not yetThis is the Symfony part oftwigphp/Twig#1913. It introduces a Twig runtime loader.Commits-------7fc174b [TwigBundle] added a Twig runtime loader
| publicfunctionload($class) | ||
| { | ||
| if (!isset($this->mapping[$class])) { | ||
| thrownew \LogicException(sprintf('Class "%s" is not configured as a Twig runtime. Add the "twig.runtime" tag to the related service in the container.',$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.
shouldn't it return null instead, to play well with multiple runtime loaders, as supported by Twig ?
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 thought about that but I don't think people will register some other runtime loaders when using Symfony. And returning null here means that debugging an issue would be very difficult.
The other possibility would be for Twig to catch exceptions and keep it for when no loaders work.
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 other possibility is to inject a logger and log such errors. WDYT?
I don't like catching the exceptions in Twig as we try to avoid that as much as possible (mainly for performance reasons).
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'm fine with logging
| $definition =$container->getDefinition('twig.runtime_loader'); | ||
| $mapping =array(); | ||
| foreach ($container->findTaggedServiceIds('twig.runtime')as$id =>$attributes) { | ||
| $mapping[$container->getDefinition($id)->getClass()] =$id; |
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.
you should validate that services are public (and not abstract), to provide an early error message for such mistake
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#20112
| <argument>%twig.form.resources%</argument> | ||
| </service> | ||
| <serviceid="twig.form.renderer"class="Symfony\Bridge\Twig\Form\TwigRenderer"public="false"> |
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.
must be public to be lazy-loadable
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.
indeed, fixed
This is the Symfony part oftwigphp/Twig#1913. It introduces a Twig runtime loader.