8000 made the assets optional by fabpot · Pull Request #13667 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

made the assets optional #13667

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

Merged
merged 1 commit into from
Feb 12, 2015
Merged

made the assets optional #13667

merged 1 commit into from
Feb 12, 2015

Conversation

fabpot
Copy link
Member
@fabpot fabpot commented Feb 12, 2015
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Right now, the assets configuration is optional (and the dependency as well on FramewokrBundle), but the asset services and the Asset extension is always loaded. This PR fixes this inconsistency, which will make tests pass again.

@fabpot fabpot merged commit ddf5ac4 into symfony:2.7 Feb 12, 2015
fabpot added a commit that referenced this pull request Feb 12, 2015
This PR was merged into the 2.7 branch.

Discussion
----------

made the assets optional

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Right now, the assets configuration is optional (and the dependency as well on FramewokrBundle), but the asset services and the Asset extension is always loaded. This PR fixes this inconsistency, which will make tests pass again.

Commits
-------

ddf5ac4 made the assets optional
@@ -78,6 +78,10 @@ public function process(ContainerBuilder $container)
$container->setDefinition('twig.loader.filesystem', $loader);
}

if ($container->has('assets.packages')) {
$loader->addTag('twig.extension');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be?

$container->getDefinition('twig.extension.assets')->addTag('twig.extension');

@stof
Copy link
Member
stof commented Feb 12, 2015

@fabpot there is a BC break here: currently the asset function is always available (and does not require any special config as it creates a default package based on the Request by default). With your change, a project which does not define asset-related settings would not get the function anymore. This means that the BC layer is incomplete

fabpot added a commit that referenced this pull request Feb 16, 2015
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0