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.

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

Demonstrate undefined index notice when using fixture dependent on ordered fixture. #198

Closed
wants to merge 1 commit into from
Closed

Demonstrate undefined index notice when using fixture dependent on ordered fixture. #198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 23 additions & 19 deletions lib/Doctrine/Common/DataFixtures/Loader.php
View file
Open in desktop
Expand Up @@ -48,7 +48,7 @@ class Loader
* @var boolean
*/
private $orderFixturesByNumber = false;

/**
* Determines if we must order fixtures by its dependencies
*
Expand Down Expand Up @@ -121,7 +121,7 @@ public function addFixture(FixtureInterface $fixture)

if (!isset($this->fixtures[$fixtureClass])) {
if ($fixture instanceof OrderedFixtureInterface && $fixture instanceof DependentFixtureInterface) {
throw new \InvalidArgumentException(sprintf('Class "%s" can\'t implement "%s" and "%s" at the same time.',
throw new \InvalidArgumentException(sprintf('Class "%s" can\'t implement "%s" and "%s" at the same time.',
get_class($fixture),
'OrderedFixtureInterface',
'DependentFixtureInterface'));
Expand Down Expand Up @@ -158,7 +158,7 @@ public function getFixtures()
if ($this->orderFixturesByDependencies) {
$this->orderFixturesByDependencies();
}

if (!$this->orderFixturesByNumber && !$this->orderFixturesByDependencies) {
$this->orderedFixtures = $this->fixtures;
}
Expand All @@ -183,7 +183,7 @@ public function isTransient($className)

/**
* Orders fixtures by number
*
*
* @todo maybe there is a better way to handle reordering
* @return void
*/
Expand All @@ -204,22 +204,22 @@ private function orderFixturesByNumber()
return 0;
});
}


/**
* Orders fixtures by dependencies
*
*
* @return void
*/
private function orderFixturesByDependencies()
{
$sequenceForClasses = array();

// If fixtures were already ordered by number then we need
// If fixtures were already ordered by number then we need
// to remove classes which are not instances of OrderedFixtureInterface
// in case fixtures implementing DependentFixtureInterface exist.
// This is because, in that case, the method orderFixturesByDependencies
// will handle all fixtures which are not instances of
// will handle all fixtures which are not instances of
// OrderedFixtureInterface
if ($this->orderFixturesByNumber) {
$count = count($this->orderedFixtures);
Expand All @@ -239,7 +239,7 @@ private function orderFixturesByDependencies()
continue;
} elseif ($fixture instanceof DependentFixtureInterface) {
$dependenciesClasses = $fixture->getDependencies();

$this->validateDependencies($dependenciesClasses);

if (!is_array($dependenciesClasses) || empty($dependenciesClasses)) {
Expand All @@ -249,7 +249,7 @@ private function orderFixturesByDependencies()
if (in_array($fixtureClass, $dependenciesClasses)) {
throw new \InvalidArgumentException(sprintf('Class "%s" can\'t have itself as a dependency', $fixtureClass));
}

// We mark this class as unsequenced
$sequenceForClasses[$fixtureClass] = -1;
} else {
Expand All @@ -261,7 +261,7 @@ private function orderFixturesByDependencies()
// Now we order fixtures by sequence
$sequence = 1;
$lastCount = -1;

while (($count = count($unsequencedClasses = $this->getUnsequencedClasses($sequenceForClasses))) > 0 && $count !== $lastCount) {
foreach ($unsequencedClasses as $key => $class) {
$fixture = $this->fixtures[$class];
Expand All @@ -270,29 +270,29 @@ private function orderFixturesByDependencies()

if (count($unsequencedDependencies) === 0) {
$sequenceForClasses[$class] = $sequence++;
}
}
}

$lastCount = $count;
}

$orderedFixtures = array();
// If there're fixtures unsequenced left and they couldn't be sequenced,

// If there're fixtures unsequenced left and they couldn't be sequenced,
// it means we have a circular reference
if ($count > 0) {
$msg = 'Classes "%s" have produced a CircularReferenceException. ';
$msg .= 'An example of this problem would be the following: Class C has class B as its dependency. ';
$msg .= 'Then, class B has class A has its dependency. Finally, class A has class C as its dependency. ';
$msg .= 'This case would produce a CircularReferenceException.';

throw new CircularReferenceException(sprintf($msg, implode(',', $unsequencedClasses)));
} else {
// We order the classes by sequence
asort($sequenceForClasses);

foreach ($sequenceForClasses as $class => $sequence) {
// If fixtures were ordered
// If fixtures were ordered
$orderedFixtures[] = $this->fixtures[$class];
}
}
Expand All @@ -303,7 +303,7 @@ private function orderFixturesByDependencies()
private function validateDependencies($dependenciesClasses)
{
$loadedFixtureClasses = array_keys($this->fixtures);

foreach ($dependenciesClasses as $class) {
if (!in_array($class, $loadedFixtureClasses)) {
throw new \RuntimeException(sprintf('Fixture "%s" was declared as a dependency, but it should be added in fixture loader first.', $class));
Expand All @@ -322,6 +322,10 @@ private function getUnsequencedClasses($sequences, $classes = null)
}

foreach ($classes as $class) {
if(!isset($sequences[$class])) {
dump('UNDEFINED INDEX');
die;
Copy link
Member

Choose a reason for hiding this comment

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

this should be removed, as it prevents merging the new test in the existing testsuite

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, without this check the notice is suppressed and the test passes. I am not sure why. This is rather demo than proper PR. Pls see the original comment.

}
if ($sequences[$class] === -1) {
$unsequencedClasses[] = $class;
}
Expand Down
50 changes: 50 additions & 0 deletions tests/Doctrine/Tests/Common/DataFixtures/DependentFixtureTest.php
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@

namespace Doctrine\Tests\Common\DataFixtures;

use Doctrine\Common\DataFixtures\AbstractFixture;
use Doctrine\Common\DataFixtures\Loader;
use Doctrine\Common\DataFixtures\DependentFixtureInterface;
use Doctrine\Common\DataFixtures\OrderedFixtureInterface;
Expand Down Expand Up @@ -169,6 +170,14 @@ public function test_inCaseGetFixturesReturnsDifferentResultsEachTime()
$this->assertInstanceOf(__NAMESPACE__ . '\BaseParentFixture1', array_shift($orderedFixtures));
$this->assertInstanceOf(__NAMESPACE__ . '\DependentFixture1', array_shift($orderedFixtures));
}

public function test_UndefinedIndexWhenFixtureDependsOnOrderedFixture()
{
$loader = new Loader();
$loader->addFixture(new MyRole2Fixture());
$orderedFixtures = $loader->getFixtures();
$this->assertCount(2, $orderedFixtures);
}
}

class DependentFixture1 implements FixtureInterface, DependentFixtureInterface
Expand Down Expand Up @@ -394,3 +403,44 @@ public function getOrder()
return 10;
}
}

abstract class MyParentFixture extends AbstractFixture
{
abstract protected function getFoo();

public function load(ObjectManager $manager)
{
$this->getFoo();
}

}


class MyRoleFixture extends MyParentFixture implements OrderedFixtureInterface
{

protected function getFoo()
{
}

public function getOrder()
{
return 1;
}
}

class MyRole2Fixture extends MyParentFixture implements DependentFixtureInterface
{

protected function getFoo()
{
}

function getDependencies()
{
return array(
'Doctrine\Tests\Common\DataFixtures\MyRoleFixture'
);
}

}