Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[3.0] use ContainerAwareTrait#16411
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
c929957 to75c0b8aCompareTobion commentedNov 1, 2015
@symfony/deciders this is ready. |
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.
we can useContainerAwareTrait in this class too
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.
good catch, changed
75c0b8a to2db4ed6Compareaitboudad commentedNov 1, 2015
👍 |
nicolas-grekas commentedNov 1, 2015
Shouldn't we deprecate in 2.8? IMHO yes, what do others think? |
Tobion commentedNov 1, 2015
@nicolas-grekas please read the description. |
wouterj commentedNov 1, 2015
I think we should deprecate in 2.8:
Actually, maybe we should even trigger a deprecation notice when PHP <5.5 in |
nicolas-grekas commentedNov 1, 2015
Of course there would be an upgrade path: first upgrade to php5.5, which is already required. I agree with@wouterj arguments (but for notices, they should not be triggered conditionally upon the php version) |
TomasVotruba commentedNov 1, 2015
👍 Awesome |
Tobion commentedNov 2, 2015
IMO developers should be able to upgrade their 2.x projects to 2.8 while avoiding any deprecation message. People stuck on PHP 5.3 would not be able to do so. |
jakzal commentedNov 3, 2015
status: reviewed 👍 |
fabpot commentedNov 4, 2015
IIRC, we agreed to not have any deprecation notices in 3.0. So, it's either 2.8 or 3.1. |
Tobion commentedNov 4, 2015
Lets just merge#16424, ill rebase this one then. |
…reTrait (nicolas-grekas)This PR was merged into the 2.8 branch.Discussion----------[DI] Deprecate ContainerAware in favor of ContainerAwareTrait| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | -To be merged before#16411 (that then should be rebased) if we agree that this is the right approach (which I believe personally).The deprecation notice will be triggered by the existing mechanism in the DebugClassLoader (it can't be added inline because that would make symfony itself trigger it).PHP 5.3 users migrating to 3.0 must already move to 2.8+5.5 beforehand so this is really on the CUP (Continuous Upgrade Path).Commits-------807ebac [DI] Deprecate ContainerAware in favor of ContainerAwareTrait
nicolas-grekas commentedNov 4, 2015
While rebasing@Tobion you could also remove the uses of the VarDumperTestCase also (replaced by VarDumperTestTrait). Or I can do it, as you want. |
Conflicts:src/Symfony/Bundle/FrameworkBundle/Controller/Controller.phpsrc/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Controller/LocalizedController.phpsrc/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Controller/LoginController.phpsrc/Symfony/Component/DependencyInjection/ContainerAware.php
2db4ed6 toe337ce6CompareI made the class final because it is not meant to be extended. By using the trait, which uses protected visibility for the container property, it would otherwise make the container property part of of BC promise
nicolas-grekas commentedNov 4, 2015
Sibling PR on 2.8 to not break 3.0 compat (and make tests green for the deps=2.8 line):#16466 |
Tobion commentedNov 4, 2015
Rebased and I found two more classes (commands in the TwigBundle) where we can use the ContainerAwareTrait. I made those commands final because they is not meant to be extended. By using the trait, which uses |
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.
a mistake that was here before but could be fixed here: the name of the file is wrong (VarDumper)
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.
done
nicolas-grekas commentedNov 4, 2015
👍 |
fabpot commentedNov 4, 2015
Tests are broken. |
nicolas-grekas commentedNov 4, 2015
The hard coded line info (63 to 63) needs to be updated (65 to 65) |
9fa2b6d tof75a940CompareTobion commentedNov 4, 2015
Fixed tests |
nicolas-grekas commentedNov 5, 2015
Thank you@Tobion. |
This PR was merged into the 3.0-dev branch.Discussion----------[3.0] use ContainerAwareTrait| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Finished version of#12595This adds the first deprecation in symfony 3. We cannot deprecate ContainerAware in 2.8 as the alternative, the trait, cannot be used with php 5.3. So there would not be an upgrade path for people using an older php versionCommits-------f75a940 [VarDumper] fix filename2309901 [VarDumper] replace VarDumperTestCase by trait24ff770 [TwigBundle] use ContainerAwareTrait in commandse337ce6 Remove deprecated ContainerAware class and make use of the trait in another class52c50e3 Remove abstract class and use Interface+Trait
Finished version of#12595
This adds the first deprecation in symfony 3. We cannot deprecate ContainerAware in 2.8 as the alternative, the trait, cannot be used with php 5.3. So there would not be an upgrade path for people using an older php version