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
Fix deprecations in fixture command #285
Conversation
@@ -38,7 +39,8 @@ public function __construct(SymfonyFixturesLoader $fixturesLoader, ?ManagerRegis | |||
), E_USER_DEPRECATED); | |||
} | |||
|
|||
parent::__construct($doctrine); | |||
// @todo The method_exists call can be removed once the DoctrineBundle dependency has been bumped to at least 1.10 |
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.
Can't you bump the dependency and release a new minor instead?
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.
That would still leave a broken version for people on older versions of DoctrineBundle, which I'd like to avoid.
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.
Fair enough, as long as this diff does get changed when merging up 👍
$doctrine = $this->getContainer()->get('doctrine'); | ||
$em = $doctrine->getManager($input->getOption('em')); | ||
// @todo The method_exists call can be removed once the DoctrineBundle dependency has been bumped to at least 1.10 | ||
if (method_exists($this, 'getDotrine')) { |
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.
getDotrine
? 😏
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.
Oh come on...
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.
Ouch... also didn't see this :S
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.
Fixed 🙄
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.
@Ocramius I'm still working on tests that manage to get to that point in the functionality. Will most likely end with completely changing the command in 4.0...
Supersedes #283.
This PR:
DoctrineCommand::__construct
on lower DoctrineBundle versionsgetContainer()
method if thegetDoctrine()
method is present.Since this is a bugfix release for 3.2, the dependency to DoctrineBundle can't really be bumped, as it will simply make this bugfix ineligible during version resolution for users of older DoctrineBundle versions. Thus, we work with
method_exists
to ensure the calls are handled correctly, removing this workaround in the next minor release where we'll properly bump the DoctrineBundle dependency.