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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
use Symfony\Component\Console\Style\SymfonyStyle; | ||
use const E_USER_DEPRECATED; | ||
use function implode; | ||
use function method_exists; | ||
use function sprintf; | ||
use function trigger_error; | ||
|
||
|
@@ -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 | ||
parent::__construct(method_exists($this, 'getDoctrine') ? $doctrine : null); | ||
|
||
$this->fixturesLoader = $fixturesLoader; | ||
} | ||
|
@@ -83,9 +85,15 @@ protected function execute(InputInterface $input, OutputInterface $output) | |
{ | ||
$ui = new SymfonyStyle($input, $output); | ||
|
||
/** @var ManagerRegistry $doctrine */ | ||
$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 commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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... |
||
$doctrine = $this->getDoctrine(); | ||
} else { | ||
/** @var ManagerRegistry $doctrine */ | ||
$doctrine = $this->getContainer()->get('doctrine'); | ||
} | ||
|
||
$em = $doctrine->getManager($input->getOption('em')); | ||
|
||
if (! $input->getOption('append')) { | ||
if (! $ui->confirm(sprintf('Careful, database "%s" will be purged. Do you want to continue?', $em->getConnection()->getDatabase()), ! $input->isInteractive())) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Doctrine\Bundle\FixturesBundle\Tests\Command; | ||
|
||
use Doctrine\Bundle\FixturesBundle\Command\LoadDataFixturesDoctrineCommand; | ||
use Doctrine\Bundle\FixturesBundle\Loader\SymfonyFixturesLoader; | ||
use Doctrine\Common\Persistence\ManagerRegistry; | ||
use PHPUnit\Framework\TestCase; | ||
use Symfony\Component\DependencyInjection\Container; | ||
|
||
class LoadDataFixturesDoctrineCommandTest extends TestCase | ||
{ | ||
/** | ||
* @group legacy | ||
* @expectedDeprecation The "Doctrine\Bundle\FixturesBundle\Command\LoadDataFixturesDoctrineCommand" constructor expects a "Doctrine\Common\Persistence\ManagerRegistry" instance as second argument, not passing it will throw a \TypeError in DoctrineFixturesBundle 4.0. | ||
*/ | ||
public function testInstantiatingWithoutManagerRegistry() : void | ||
{ | ||
$loader = new SymfonyFixturesLoader(new Container()); | ||
|
||
new LoadDataFixturesDoctrineCommand($loader); | ||
} | ||
|
||
/** | ||
* @doesNotPerformAssertions | ||
*/ | ||
public function testInstantiatingWithManagerRegistry() : void | ||
{ | ||
$registry = $this->createMock(ManagerRegistry::class); | ||
$loader = new SymfonyFixturesLoader(new Container()); | ||
|
||
new LoadDataFixturesDoctrineCommand($loader, $registry); | ||
} | ||
} |
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 👍