[FrameworkBundle] Enable assets by default #13672
Conversation
|
Given that this is BC break (the change I made earlier today), I think the better fix is to add |
|
Does it also mean that assets should be loaded automatically, without the need to enable them in the config? |
|
@jakzal it means that they should be enabled by default in the FrameworkBundle config (we could keep the possibility to disable them explicitly if you don't want the asset system, but I'm not sure it is worth it given it does not add overhead but only defines a few services IIRC) |
|
Well, I fear that the answer is yes, to keep BC. We might revisit this decision in 3.0 though. |
|
Cool. I'll update this PR. |
6599bf3 to
2fb59f3
Compare
|
What about disabling all components by default in symfony 3.0. |
f1d40f0 to
3808735
Compare
|
This is ready. |
|
@Tobion 👍 I'm in favor of disabling everything by default. |
| @@ -53,6 +53,14 @@ public function getConfigTreeBuilder() | |||
| return $v; | |||
| }) | |||
| ->end() | |||
| ->beforeNormalization() | |||
There was a problem hiding this comment.
I think this should rather be done on validate. Otherwise, you will create issues if 1 file uses the old syntax, because you will inject the new syntax in all other config files being merged
There was a problem hiding this comment.
Good spot. Unfortunately, in validate I'll need to explicitly provide assets config.
There was a problem hiding this comment.
indeed. But it is better than a broken implementation
There was a problem hiding this comment.
Obviously ;)
I just pushed the change.
3808735 to
e32e464
Compare
| @@ -84,6 +84,14 @@ public function getConfigTreeBuilder() | |||
| 'base_urls' => array_values(array_unique(array_merge($config['base_urls']['http'], $config['base_urls']['ssl']))), | |||
| ); | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
this is the wrong place to set it (you are enabling it only when templating is set, while it should always be set)
There was a problem hiding this comment.
Right. That's why I initially added it in a separate section in the first place.
There was a problem hiding this comment.
Wait, but isn't templating always set at this point?
There was a problem hiding this comment.
Answering my own question: not necessarily.
We need to enable assets if they're not enabled, but only if templating doesn't contain asset configuration.
PR Updated.
e32e464 to
059ec7c
Compare
059ec7c to
f5c0a69
Compare
|
Indeed, having everything disabled by default would be better; and Symfony SE can enable the most important things and add comments to enable less important features. Anyone willing to work on this for 3.0? |
|
Thank you @jakzal. |
This PR was merged into the 2.7 branch. Discussion ---------- [FrameworkBundle] Enable assets by default | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13667 | License | MIT | Doc PR | - This should make the tests pass again. There are two templates from TwigBundle that are used with functional tests, and require the asset() helper: * src/Symfony/Bundle/TwigBundle/Resources/views/Exception/exception_full.html.twig * src/Symfony/Bundle/TwigBundle/Resources/views/layout.html.twig Commits ------- f5c0a69 [FrameworkBundle] Enable assets by default.
|
Created issue #13703 to track this |
This should make the tests pass again.
There are two templates from TwigBundle that are used with functional tests, and require the asset() helper: