[TwigBundle] Refresh twig paths when resources change. #14262
Conversation
aitboudad
commented
Apr 7, 2015
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Fixed tickets | ~ |
| Tests pass? | yes |
| License | MIT |
| foreach ($container->getParameter('kernel.bundles') as $bundle => $class) { | ||
| if (is_dir($dir = $container->getParameter('kernel.root_dir').'/Resources/'.$bundle.'/views')) { | ||
| $this->addTwigPath($twigFilesystemLoaderDefinition, $dir, $bundle); | ||
| $dirs[] = $dir; |
There was a problem hiding this comment.
Why don't we call addResource() here directly?
There was a problem hiding this comment.
just to avoid call $container->addResource(new DirectoryResource($dir)); in several places
There was a problem hiding this comment.
fixed 2 vs 1 :), seem there is not big difference but I still like the old way.
ac51644 to
2462bc2
Compare
|
👍 |
|
Why do you consider this as a new feature? Looks like a bug to me, no? |
|
@fabpot I think so as it throw an exception when we remove an already registed path, you can merge it into |
|
Thank you @aitboudad. |
…tboudad) This PR was submitted for the 2.7 branch but it was merged into the 2.3 branch instead (closes #14262). Discussion ---------- [TwigBundle] Refresh twig paths when resources change. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Fixed tickets | ~ | Tests pass? | yes | License | MIT Commits ------- cafb0d7 [TwigBundle] Refresh twig paths when resources change.
|
Hello, This commit has a huge (+900% on a simple login page) impact on performances when the resources are being tracked by the container. In fact, I don't understand why the Resources/view directories are added to the tracked resources ? Regards. |
|
@chbruyand because we need to reconfigure twig when the directory does not exist anymore. @aitboudad @fabpot I think we actually need a @chbruyand does this have an impact on all page load or only the first time after the symfony update ? If it is only the first time, it might be becaus eyou compare a request rebuilding the cache with a request not rebuilding it. |
|
@stof this is on all requests being made, not only when building the container for the first time. |
|
@stof Do I get you right that in this case the contents of the directory do not matter, just the fact that a particular directory does or does not exist with the resource being stale if that changes? |
|
I updated my dev-environment to symfony 2.3.29 which seems to contain this patch and it really seems to have a performance penalty in the figures that @chbruyand pointed out. What makes it worse is that the site I'm writing makes additional ajax requests to the symfony-backend after the initial page-load and each one of the seems to be impacted in the same way. I don't know if there's issues with my environment configuration, but going from .28 to .29 really broke the performance for me in dev-environment and these changes seem to be at the bottom of it for me. |
|
@stof I was starting to work on a patch based on your thoughts. Using a FileResource instead of a DirectoryResource would ensure twig is reconfigured when directory is deleted (I think there is no need for a DirectoryExistenceResource there). If we really want to skip the modification time check in that case, I would argue in favour of a FileExistenceResource or an extra argument to the existing FileResource. |
|
@chbruyand we would also need to have it reconfigured when the non-existent directory is added though |
|
And yes, it can be named |
* 2.3: Revert "bug #14262 [TwigBundle] Refresh twig paths when resources change. (aitboudad)" InvalidResourceException file name [Validators] Remove forgotten space in a translation key [nl] [Validators] Correct translation key and content [nl] added missing CVE number bumped Symfony version to 2.3.30 updated VERSION for 2.3.29 update CONTRIBUTORS for 2.3.29 updated CHANGELOG for 2.3.29 [CS] [Console] StreamOuput : fix loose comparison [DependencyInjection] Avoid unnecessary calls to strtolower() Conflicts: src/Symfony/Component/Console/Output/StreamOutput.php src/Symfony/Component/HttpKernel/Kernel.php
* 2.6: (21 commits) Revert "bug #14262 [TwigBundle] Refresh twig paths when resources change. (aitboudad)" InvalidResourceException file name [Validators] Remove forgotten space in a translation key [nl] [Validators] Correct translation key and content [nl] bumped Symfony version to 2.6.9 updated VERSION for 2.6.8 updated CHANGELOG for 2.6.8 added missing CVE number bumped Symfony version to 2.3.30 updated VERSION for 2.3.29 update CONTRIBUTORS for 2.3.29 updated CHANGELOG for 2.3.29 [Validators] Missing translations for arabic language. Code style fixed C [HttpKernel][Bundle] Check extension implements ExtensionInterface [DebugBundle] Fix config XSD [CS] [Console] StreamOuput : fix loose comparison [Framework][router commands] fixed failing test. [HttpKernel] Do not call the FragmentListener if _controller is already defined ... Conflicts: src/Symfony/Component/HttpKernel/Kernel.php src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php
…ted (chbruyand) This PR was squashed before being merged into the 2.8 branch (closes #14781). Discussion ---------- [TwigBundle] Reconfigure twig paths when they are updated | Q | A | ------------- | --- | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14771, #14768, #14262, #14778 | License | MIT Refresh twig paths upon creation and deletion. As we don't care neither about path's modification time nor path's content, a new Resource has been added in the Config Component. Full discussion in #14778. Commits ------- 3cbff05 [TwigBundle] Reconfigure twig paths when they are updated