-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Updated configuration for the new Asset component #5170
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
Updated configuration for the new Asset component #5170
Conversation
javiereguiluz
commented
Apr 13, 2015
Q | A |
---|---|
Doc fix? | yes |
New docs? | no |
Applies to | 2.7+ |
Fixed tickets | #4982 (partially) |
@@ -50,7 +50,7 @@ Full Default Configuration | |||
some_filter: [] | |||
workers: | |||
# see https://github.com/symfony/AsseticBundle/pull/119 | |||
# Cache can also be busted via the framework.templating.assets_version | |||
# Cache can also be busted via the framework.assets.version |
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.
do you think a note should be added here as well? https://github.com/symfony/symfony/blob/2.7/UPGRADE-2.7.md I found a lot of changes are passing unreported.
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.
@cordoval UPGRADE file should only contain breaking changes, CHANGELOG files of each component/bundle/bridge will contain all changes (including non-BC changes like this).
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 guess i have misunderstood what breaking means then. This definitely broke his system 👴. But yeah it is not i guess in the BC promise?
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 weird, as there is a BC layer: https://github.com/symfony/FrameworkBundle/blob/2.7/DependencyInjection/Configuration.php#L79-113 (triggering a deprecation notice is not breaking)
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.
@cordoval Can you give an example how this broke a system?
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.
there is a ticket on symfony/symfony about this that I referenced it symfony/symfony#14332
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.
Well, that's no breaking change, but is a misconfiguration of the templating system.
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.
correct, but i see this happening more often, things are a bit cryptic for the people that is migrating to 2.7 since they have to be savvy in debugging things like this. I am cool either way, just pointing things.
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.
Actually, I agree - this should have been in the UPGRADE log (the change of the configuration). Strictly speaking, there are no BC breaks on minor version changes, so the UPGRADE log is for exactly stuff like this - big changes where some old way of doing something still works, but is deprecated (check out the Form section of UPGRADE-2.7.md). I've seen a few people complain about the asset stuff being missing from UPGRADE.
you should also add the documentation for the new |
@@ -417,33 +417,46 @@ Whether to enable the ``serializer`` service or not in the service container. | |||
|
|||
For more details, see :doc:`/cookbook/serializer`. | |||
|
|||
templating | |||
~~~~~~~~~~ | |||
assets |
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 think we should add a label for the old templating
key to help people finding the new options.
fccd261
to
690b6d5
Compare
* `hinclude_default_template`_ | ||
* :ref:`form <reference-templating-form>` | ||
* `resources`_ | ||
* `assets_base_urls`_ |
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.
It'd be nice to make sure these #anchor
links still work (e.g. http://symfony.com/doc/current/reference/configuration/framework.html#assets-version). You can do this by adding a reference above the new, equivalent section:
.. _assets-version:
version
-------
Hey Javier! I'd like to merge this, but since you started it, we had a big overhaul of this config chapter, and I think the conflict is basically unfixable - even for me (who merged that other big PR). We might need to "re-do" these changes on a fresh 2.7 branch. Can you try that? Thanks! |
@javiereguiluz can you also add some examples of |
Closing it in favor of #5390. |