Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[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 to75c0b8aCompare@symfony/deciders this is ready. |
| /** | ||
| * A simple implementation of ContainerAwareInterface. | ||
| * | ||
| * @author Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * @deprecated since version 3.0, to be removed in 4.0. Use ContainerAwareTrait with ContainerAwareInterface instead. | ||
| */ | ||
| abstract class ContainerAware implements ContainerAwareInterface |
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 to2db4ed6Compare👍 |
Shouldn't we deprecate in 2.8? IMHO yes, what do others think? |
@nicolas-grekas please read the description. |
I think we should deprecate in 2.8:
Actually, maybe we should even trigger a deprecation notice when PHP <5.5 in |
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) |
👍 Awesome |
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. |
status: reviewed 👍 |
IIRC, we agreed to not have any deprecation notices in 3.0. So, it's either 2.8 or 3.1. |
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
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
Sibling PR on 2.8 to not break 3.0 compat (and make tests green for the deps=2.8 line):#16466 |
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 |
| use Symfony\Component\VarDumper\Test\VarDumperTestTrait; | ||
| class VarDumperTestTraitTest extends \PHPUnit_Framework_TestCase |
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
👍 |
Tests are broken. |
The hard coded line info (63 to 63) needs to be updated (65 to 65) |
9fa2b6d tof75a940CompareFixed tests |
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