Skip to content

[DependencyInjection] register alias after defining the class #21096

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 1 commit into from
Dec 29, 2016
Merged

[DependencyInjection] register alias after defining the class #21096

merged 1 commit into from
Dec 29, 2016

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Dec 29, 2016

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

@nicolas-grekas
Copy link
Member

renaming is hard :)
👍

@xabbuh
Copy link
Member Author

xabbuh commented Dec 29, 2016

Maybe we should add the process to the docs. :D

Jean85
Jean85 suggested changes Dec 29, 2016
View reviewed changes
Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Why not use my latest approach? 1e29773

In this way, the file that is getting deprecated will contain the class_alias, and will be deleted altogether in next major, without touching the new class

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 29, 2016 •
edited
Loading

Because 1e29773 doesn't work: the alias must be defined alongside the aliased class so that instanceof checks work as expected.

@Jean85
Copy link
Contributor

Jean85 commented Dec 29, 2016

But @nicolas-grekas the change does work! See the build status in my PR #21088 from which I extracted the prev. commit.

If you look at 41f995e I've added new assertion to the test to see that the instanceof works for both class definitions, the old (deprecated) and the new; it should work since the autoloader will attempt to load the old definition, finding the old file, which contains the class_alias, hence making it work.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 29, 2016 •
edited
Loading

It works because you already loaded everything.
The broken scenario is:
load TraceableAccessDecisionManager (but NOT DebugAccessDecisionManager)
check an instance of it against DebugAccessDecisionManager

@fabpot
Copy link
Member

fabpot commented Dec 29, 2016

Thank you @xabbuh.

@fabpot fabpot merged commit d8d4417 into symfony:master Dec 29, 2016
fabpot added a commit that referenced this pull request Dec 29, 2016
@fabpot
minor #21096 [DependencyInjection] register alias after defining the …
ba932ae 
…class (xabbuh)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DependencyInjection] register alias after defining the class

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

Commits
-------

d8d4417 [DI] register alias after defining the class
@xabbuh xabbuh deleted the pr-20967 branch December 29, 2016 23:05
@Jean85
Copy link
Contributor

Jean85 commented Dec 30, 2016

@nicolas-grekas nope. As you can see, I've added your case too here: 78c8e08
And the build is green: https://travis-ci.org/symfony/symfony/builds/187649329

@Jean85 Jean85 mentioned this pull request Dec 30, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jean85 Jean85 Jean85 requested changes

Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@xabbuh @nicolas-grekas @Jean85 @fabpot @carsonbot