-
Notifications
You must be signed in to change notification settings - Fork 76
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
Change the way to inject ActiveTheme since Symfony 3.2.1 #159
Conversation
In Symfonny 3.2.1 a new parameter has been added to the TwigBundle FilesystemLoader breaking the LiipThemeBundle own FilesystemLoader. Instead of adding a 4th parameter, we define the service to call a method after being instanciated to define the ActiveTheme.
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 should be safe to change constructor because the definition of twig.loader.filesystem
is compiled using ComplierPass
env: SYMFONY_VERSION=3.1.* | ||
- php: 5.6 | ||
env: SYMFONY_VERSION=3.2.* | ||
- php: 5.6 |
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.
How about adding @latest
also?
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.
By that, you mean the dev-master
?
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.
Yes
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 build fail with dev-master
.
I guess it's because it might be harder to resolve deps when it's the master branch.
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.
Can't reproduce this locally event with composer version b1156ed376adc39de57f4bb4ed2c345ae8cbeb7e (snapshot-channel). Try adding this: composer self-update --stable
Profile output:
[468.7MB/73.19s] - Installing symfony/symfony (dev-master 9f95654)[468.7MB/174.70s] Cloning 9f95654515 from cache
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.
Looks like composer is already updated to the latest stable release:
Updating to version 1.2.4 (stable channel).
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's not 1.2.4. See https://travis-ci.org/liip/LiipThemeBundle/jobs/184169726#L151
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 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.
Sorry :) missed that
we at sulu has also this problem when we try to upgrade to symfony3 and with this fix everything works fine with the this bundle. |
{ | ||
parent::__construct(array()); | ||
parent::__construct(array(), $rootPath); |
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.
would there be any sense in making the rootpath configurable?
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 mean, in the compiler pass?
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.
fundamentally via Bundle config
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.
kernel.root_dir
is injected there via ExtensionPass
from symfony/symfony#20727
do you think bundle should override 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.
ah no .. thx for checking
In Symfonny 3.2.1 a new parameter has been added to the TwigBundle FilesystemLoader breaking the LiipThemeBundle own FilesystemLoader.
Instead of adding a 4th parameter, we define the service to call a method after being instanciated to define the ActiveTheme.
Related:
I don't know if this provide a BC break since we are changing the constructor definition..