Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 5, 2015
Merged

[3.0] use ContainerAwareTrait #16411

merged 5 commits into from
Nov 5, 2015

Conversation

Tobion
Copy link
Contributor

@Tobion Tobion commented Nov 1, 2015

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 #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

@Tobion Tobion force-pushed the container-aware-trait branch from c929957 to 75c0b8a Compare November 1, 2015 16:40
@Tobion Tobion mentioned this pull request Nov 1, 2015
Closed
@Tobion
Copy link
Contributor Author

Tobion commented Nov 1, 2015

@symfony/deciders this is ready.

aitboudad
aitboudad reviewed Nov 1, 2015
View reviewed changes
src/Symfony/Component/DependencyInjection/ContainerAware.php
/**
* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use ContainerAwareTrait in this class too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, changed

@Tobion Tobion force-pushed the container-aware-trait branch from 75c0b8a to 2db4ed6 Compare November 1, 2015 16:59
@aitboudad
Copy link
Contributor

👍

@nicolas-grekas
Copy link
Member

Shouldn't we deprecate in 2.8? IMHO yes, what do others think?

@Tobion
Copy link
Contributor Author

Tobion commented Nov 1, 2015

@nicolas-grekas please read the description.

@wouterj
Copy link
Member

wouterj commented Nov 1, 2015

I think we should deprecate in 2.8:

  • Symfony 3 requires PHP 5.5
  • Symfony promises a very smooth (no changes required) upgrade process from 2.8 to 3.0 when having no deprecation notices in 2.8
  • This means applications have to run Symfony 2.8 on PHP 5.5 already
  • Which means they can use the trait (as it's available from PHP 5.4)

Actually, maybe we should even trigger a deprecation notice when PHP <5.5 in Kernel#handle() or the like in Symfony 2.8.

@nicolas-grekas
Copy link
Member

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
Copy link
Contributor

👍 Awesome

@Tobion
Copy link
Contributor Author

Tobion commented Nov 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.
Anyway, I'm ok with whatever approach we take. Time for @fabpot to decide :)

@nicolas-grekas nicolas-grekas mentioned this pull request Nov 2, 2015
Merged
@jakzal
Copy link
Contributor

jakzal commented Nov 3, 2015

status: reviewed

👍

@fabpot
Copy link
Member

fabpot commented Nov 4, 2015

IIRC, we agreed to not have any deprecation notices in 3.0. So, it's either 2.8 or 3.1.

@Tobion
Copy link
Contributor Author

Tobion commented Nov 4, 2015

Lets just merge #16424, ill rebase this one then.

fabpot added a commit that referenced this pull request Nov 4, 2015
@fabpot
feature #16424 [DI] Deprecate ContainerAware in favor of ContainerAwa…
a594b9f 
…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
Copy link
Member

While rebasing @Tobion you could also remove the uses of the VarDumperTestCase also (replaced by VarDumperTestTrait). Or I can do it, as you want.

blanchonvincent and others added 2 commits November 4, 2015 18:03
@blanchonvincent @Tobion
Remove abstract class and use Interface+Trait
52c50e3 
Conflicts:
	src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php
	src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Controller/LocalizedController.php
	src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Controller/LoginController.php
	src/Symfony/Component/DependencyInjection/ContainerAware.php
@Tobion
Remove deprecated ContainerAware class and make use of the trait in a…
e337ce6 
…nother class
@Tobion Tobion force-pushed the container-aware-trait branch from 2db4ed6 to e337ce6 Compare November 4, 2015 17:12
@nicolas-grekas nicolas-grekas mentioned this pull request Nov 4, 2015
Closed
@Tobion
[TwigBundle] use ContainerAwareTrait in commands
24ff770 
I 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
Copy link
Member

Sibling PR on 2.8 to not break 3.0 compat (and make tests green for the deps=2.8 line): #16466

@Tobion
Copy link
Contributor Author

Tobion commented Nov 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 protected visibility for the container property, it would otherwise make the container property part of of BC promise.
I also changed the VarDumper to use its trait.

nicolas-grekas
nicolas-grekas reviewed Nov 4, 2015
View reviewed changes
src/Symfony/Component/VarDumper/Tests/Test/VarDumpTestTraitTest.php

use Symfony\Component\VarDumper\Test\VarDumperTestTrait;

class VarDumperTestTraitTest extends \PHPUnit_Framework_TestCase
Copy link
Member

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Tobion Tobion changed the title [3.0] use ContainerAwareTrait and deprecate ContainerAware class [3.0] use ContainerAwareTrait Nov 4, 2015
@nicolas-grekas
Copy link
Member

👍

@fabpot
Copy link
Member

fabpot commented Nov 4, 2015

Tests are broken.

@nicolas-grekas
Copy link
Member

The hard coded line info (63 to 63) needs to be updated (65 to 65)

@Tobion Tobion force-pushed the container-aware-trait branch from 9fa2b6d to f75a940 Compare November 4, 2015 23:39
@Tobion
Copy link
Contributor Author

Tobion commented Nov 4, 2015

Fixed tests

@nicolas-grekas
Copy link
Member

Thank you @Tobion.

@nicolas-grekas nicolas-grekas merged commit f75a940 into symfony:master Nov 5, 2015
nicolas-grekas added a commit that referenced this pull request Nov 5, 2015
@nicolas-grekas
feature #16411 [3.0] use ContainerAwareTrait (blanchonvincent, Tobion)
697e6da 
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 #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

Commits
-------

f75a940 [VarDumper] fix filename
2309901 [VarDumper] replace VarDumperTestCase by trait
24ff770 [TwigBundle] use ContainerAwareTrait in commands
e337ce6 Remove deprecated ContainerAware class and make use of the trait in another class
52c50e3 Remove abstract class and use Interface+Trait
@Tobion Tobion deleted the container-aware-trait branch November 5, 2015 11:35
@fabpot fabpot mentioned this pull request Nov 16, 2015
Merged
@teohhanhui teohhanhui mentioned this pull request Mar 25, 2016
Closed
@craigh craigh mentioned this pull request Nov 1, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Tobion @aitboudad @nicolas-grekas @wouterj @TomasVotruba @jakzal @fabpot @carsonbot @blanchonvincent