Skip to content
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

Merged
merged 3 commits into from Jun 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 12 additions & 4 deletions Command/LoadDataFixturesDoctrineCommand.php
Expand Up @@ -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;

Expand All @@ -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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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 👍

parent::__construct(method_exists($this, 'getDoctrine') ? $doctrine : null);

$this->fixturesLoader = $fixturesLoader;
}
Expand Down Expand Up @@ -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')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

getDotrine? 😏

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh come on...

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed 🙄

Copy link
Member Author

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

$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())) {
Expand Down
36 changes: 36 additions & 0 deletions Tests/Command/LoadDataFixturesDoctrineCommandTest.php
@@ -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);
}
}
4 changes: 4 additions & 0 deletions phpunit.xml.dist
Expand Up @@ -17,4 +17,8 @@
</exclude>
</whitelist>
</filter>

<listeners>
<listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener"/>
</listeners>
</phpunit>