-
-
Notifications
You must be signed in to change notification settings - Fork 388
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
Set dependencyFactory before configure is called in DoctrineCommand #1043
Conversation
Nice find! Can you please add a test here: https://github.com/doctrine/migrations/blob/3.0.x/tests/Doctrine/Migrations/Tests/Tools/Console/Command/DoctrineCommandTest.php ? |
3d22bc1
to
574acbf
Compare
@greg0ire I added a test case and made sure it fails without my change. Please take a look at it. And thank you for your patience. |
tests/Doctrine/Migrations/Tests/Tools/Console/Command/DoctrineCommandTest.php
Outdated
Show resolved
Hide resolved
Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble. How to do that?
|
Rebased and squashed. |
tests/Doctrine/Migrations/Tests/Tools/Console/Command/DoctrineCommandTest.php
Outdated
Show resolved
Hide resolved
f3cbfed
to
60a015d
Compare
@SenseException I have rebased it and added the |
tests/Doctrine/Migrations/Tests/Tools/Console/Command/DoctrineCommandTest.php
Outdated
Show resolved
Hide resolved
@SenseException makes sense! code updated |
thanks everyone! |
Summary
While reading through the code I found this piece here and I figured, it would never actually do anything with the way things are set up. The constructor always sets the value for
$dependencyFactory
only after calling its parent, so until then$dependencyFactory
is considerednull
. The parent's constructor in turn calls theconfigure
method which checks if$dependencyFactory
isnull
.I have switched the assignment of
$dependencyFactory
with the call to the parent's constructor, so that the condition in theconfigure
method actually does something depending on the presence of a$dependencyFactory
.I consider this a breaking change, since this can potentially lead to one less input option for a doctrine command, thus changing the command definition and breaking scripts that provide this option.