New ProvideFixtureInterface #74

Closed
wants to merge 13 commits into
from

5 participants

@djlambert

I was thinking it would be much easier to define fixture dependencies than manually setting an order. After looking at the code I found there was an existing DependentFixtureInterface that was undocumented. The new ProvideFixtureInterface allows the use of a simple string instead of the full fixture class name in the array returned from getDependencies. Documentation is also included.

The test ProxyReferenceRepositoryTest::testReferenceReconstruction is failing, but it's also failing on master.

@stof
Doctrine member

is this new ProvidesFixtureInterface really needed ? when you depend on classes in the same namespace, you can use _NAMESPACE__ to avoid duplicating the namespace, and it avoids issues. Once you start adding a ProvidesFixtureInterface, you also need to add some checks about the uniqueness of the provided alias to avoid ambiguity (which is not the case in your patch and will lead to unexpected behaviors).

Could you send the documentation for the existing feature in a separate pull request so that it can be merged separately ?

@djlambert

In the project I'm working on I've defined the entities in namespaces according to inheritance or relationships like:

MyNamespace\Media
MyNamespace\Media\Audio
MyNamespace\Media\Video
MyNamespace\User

I've setup my fixtures to follow the same scheme

MyFixtures\LoadMedia
MyFixtures\Media\LoadAudio
MyFixtures\Media\LoadVideo
MyFixtures\LoadUser

So in LoadAudio I can't use __NAMESPACE__ to reference LoadUser. I did add a check in Loader::addFixture to make sure the alias wasn't already defined. Are there additional checks that would be needed?

Initially I was going to create a base fixture with the aliases and then have my fixtures extend that, but I thought others might find the feature useful and added the interface.

@guilhermeblanco
Doctrine member

Please rebase your PR since you have 2 different requests here (docs and code changes).
I merged the documentation one, but to move this one further, you need to rebase it.

@djlambert

Rebased.

@stof stof commented on an outdated diff Sep 13, 2012
lib/Doctrine/Common/DataFixtures/Loader.php
@@ -140,6 +147,24 @@ public function addFixture(FixtureInterface $fixture)
$this->orderFixturesByDependencies = true;
}
+ if ($fixture instanceof ProvidesFixtureInterface) {
+ $fixtureProvides = $fixture->getProvides();
+ if (!empty($fixtureProvides) && is_string($fixtureProvides)) {
+ if (isset($this->fixtureProvides[$fixtureProvides])) {
+ throw new \InvalidArgumentException(sprintf('Class "%s" can\'t provide "%s", it is already provided by "%s".',
+ $fixtureClass,
+ $fixtureProvides,
+ $this->fixtureProvides[$fixtureProvides]));
@stof
Doctrine member
stof added a line comment Sep 13, 2012

Please fix the indentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Sep 13, 2012
lib/Doctrine/Common/DataFixtures/Loader.php
@@ -140,6 +147,24 @@ public function addFixture(FixtureInterface $fixture)
$this->orderFixturesByDependencies = true;
}
+ if ($fixture instanceof ProvidesFixtureInterface) {
+ $fixtureProvides = $fixture->getProvides();
+ if (!empty($fixtureProvides) && is_string($fixtureProvides)) {
+ if (isset($this->fixtureProvides[$fixtureProvides])) {
+ throw new \InvalidArgumentException(sprintf('Class "%s" can\'t provide "%s", it is already provided by "%s".',
+ $fixtureClass,
+ $fixtureProvides,
+ $this->fixtureProvides[$fixtureProvides]));
+ } else {
+ $this->fixtureProvides[$fixture->getProvides()] = $fixtureClass;
@stof
Doctrine member
stof added a line comment Sep 13, 2012

why not reusing the variable ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Sep 13, 2012
lib/Doctrine/Common/DataFixtures/Loader.php
@@ -140,6 +147,24 @@ public function addFixture(FixtureInterface $fixture)
$this->orderFixturesByDependencies = true;
}
+ if ($fixture instanceof ProvidesFixtureInterface) {
+ $fixtureProvides = $fixture->getProvides();
+ if (!empty($fixtureProvides) && is_string($fixtureProvides)) {
@stof
Doctrine member
stof added a line comment Sep 13, 2012

please use the opposite condition and throw the exception, which then avoids uisng else for the other case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Sep 13, 2012
...rine/Common/DataFixtures/ProvidesFixtureInterface.php
+/**
+ * ProvidesFixtureInterface can be implemented to use
+ * string values instead of class names by fixtures to
+ * depend on other fixtures
+ *
+ * @author Derek J. Lambert <dlambert@dereklambert.com>
+ */
+interface ProvidesFixtureInterface
+{
+ /**
+ * This method must return a string representing what the
+ * fixture provides to dependent classes
+ *
+ * @return string
+ */
+ public function getProvides();
@stof
Doctrine member
stof added a line comment Sep 13, 2012

the naming is weird as it returns a single alias for the fixture class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Sep 13, 2012
...ine/Tests/Common/DataFixtures/ProvidesFixtureTest.php
+ *
+ * @author Derek J. Lambert <dlambert@dereklambert.com>
+ */
+class ProvideDependentFixtureTest extends BaseTest
+{
+ public function testOrderFixturesByProvidesDependencies()
+ {
+ $loader = new Loader();
+ $loader->addFixture(new ProvidesFixture4);
+ $loader->addFixture(new ProvidesFixture2);
+ $loader->addFixture(new ProvidesFixture3);
+ $loader->addFixture(new ProvidesFixture1);
+
+ $orderedFixtures = $loader->getFixtures();
+
+ $this->assertEquals(4, count($orderedFixtures));
@stof
Doctrine member
stof added a line comment Sep 13, 2012

you should use assertCount

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Sep 13, 2012
...ine/Tests/Common/DataFixtures/ProvidesFixtureTest.php
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\Tests\Common\DataFixtures;
+
+use Doctrine\Common\DataFixtures\Loader;
+use Doctrine\Common\DataFixtures\ProvidesFixtureInterface;
+use Doctrine\Common\DataFixtures\DependentFixtureInterface;
+use Doctrine\Common\DataFixtures\OrderedFixtureInterface;
+use Doctrine\Common\DataFixtures\FixtureInterface;
+use Doctrine\Common\Persistence\ObjectManager;
+
+require_once __DIR__.'/TestInit.php';
@stof
Doctrine member
stof added a line comment Sep 13, 2012

IIRC, this is not needed anymore

@djlambert
djlambert added a line comment Sep 13, 2012

Which part? I've removed OrderedFixtureInterface.

@stof
Doctrine member
stof added a line comment Sep 13, 2012

The require_once call, which is the line on which I commented

@djlambert
djlambert added a line comment Sep 14, 2012

Sorry, I was thinking the comment applied to the whole section of code. I didn't realize the last line was the one being commented on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Sep 13, 2012
...ine/Tests/Common/DataFixtures/ProvidesFixtureTest.php
+ */
+class ProvideDependentFixtureTest extends BaseTest
+{
+ public function testOrderFixturesByProvidesDependencies()
+ {
+ $loader = new Loader();
+ $loader->addFixture(new ProvidesFixture4);
+ $loader->addFixture(new ProvidesFixture2);
+ $loader->addFixture(new ProvidesFixture3);
+ $loader->addFixture(new ProvidesFixture1);
+
+ $orderedFixtures = $loader->getFixtures();
+
+ $this->assertEquals(4, count($orderedFixtures));
+
+ $this->assertTrue(array_shift($orderedFixtures) instanceof ProvidesFixture1);
@stof
Doctrine member
stof added a line comment Sep 13, 2012

you should use assertInstanceOf instead of assertTrue. It gives a better error message when failing than Failed asserting that boolean<false> is true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterhorne

@djlambert I was just thinking that this feature would be really useful, thanks! Hope to see this PR accepted soon :)

@lavoiesl
Doctrine member

It seems that this PR is obsolete since we now use DependentFixtureInterface, can we close this?

@lavoiesl lavoiesl closed this Mar 23, 2015
@lavoiesl lavoiesl added the Duplicate label Mar 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment