Data Fixtures version 2.0

[guilhermeblanco]

Hi guys,

Here is an initial draft for evaluation for the newer version of our data fixtures code. I'd like people to evaluate and ask whatever questions you may have on this ticket. It is not fully planned, like DependentCalculator requires a Topological sorting to prioritize which fixtures should be loaded first. Same thing for ORMPurger. Another missing piece is the wish to use Iterators for file loading. All that aside, the overall engineer of the tool is planned. Please evaluate and provide feedback.

Coding should start in a few days after everybody is satisfied with planned solution. I may update the diagrams during discussions on comments.

Cheers,

Guilherme Blanco

Doctrine\Fixture

Doctrine\Fixture\Executor

Doctrine\Fixture\Loader

Doctrine\Fixture\Purger

Doctrine\Fixture\Reference

Doctrine\Fixture\Sorter\Calculator

Doctrine\Fixture\Sorter\Type

[guilhermeblanco]

@asm89 @beberlei @jwage @FabioBatSilva @jmikola @ocramius @stof @fabpot @schmittjoh I would love to hear some comments from you guys about this new plan. =)

[Ocramius]

As discussed on IRC, there's still this weird parallelism between connection and manager registries... Is there some way of getting rid of it?

Also: is the reason why the various executors and purgers have access to the registries? If it's for lazy loading, then it's not their responsibility IMO :)

Good work!

[stof]

@Ocramius the ManagerRegistry interface extends from ConnectionRegistry IIRC.

[stof]

and the executor needs to pass the object manager to the fixture it executes, so it needs to be able to retrieve it

@guilhermeblanco in such a setup, where would you handle the ContainerAwareInterface of Symfony. Currently, it is done in the loader but it does not fit well with your new design with several loaders.

[Ocramius]

@stof yeah, but there's still two times the methods... Of course connections are no managers, but they're usually coupled

[stof]

@Ocramius the great thing about the current interface is that getManager always returns an ObjectManager, not a mixed return value

[Ocramius]

@stof wouldn't expect getManager to return mixed of course :) mixed is a forbidden sin.

About the ContainerAwareInterface, I'd say that could be implemented on the registries in the bundles: registries simply bridge to the DIC then.

[stof]

@Ocramius registries have nothing to do with fixtures (they are not even part of the DataFixtures library). So this is irrelevant

[Ocramius]

So you'd want to have fixtures themselves implement ContainerAwareInterface somehow? That could be handled by a custom DIC based loader...

[stof]

@Ocramius this is what the current ContainerAwareLoader does. See the linked code.

Using a custom loader seems wrong to me in the new design. It is not a different way to load the fixture classes. It is an action applied on all fixtures if they implement the interface, after they are instantiated but before they are executed. We would have to extend all loaders (which is what we do currently except there is only 1 loader implementation in the library).

[Ocramius]

@stof aww... Yes, doesn't seem really nice, but on the other side it's not that bad (consider that I totally hate the initializer pattern for services... I'm biased).

If you want fixtures as services:

1. define a DIC based loader (instead of directory/glob/class loaders)
2. eventually consider using the calculator to initialize the DIC aware fixtures

[schmittjoh]

Why not simply use composition for the DIC loader?

@guilhermeblanco, do you have some before/after analysis, or the main goals of the refactoring?

[stof]

@Ocramius I don't want to define my fixtures as service (though we could also provide such a loader in the new system for people wanting them). I want to be able to use my services in the fixtures (to avoid doing lots of code duplication)

[Ocramius]

@stof I would totally encourage people wanting fixtures as services to do so instead of using initializers... They are always a bit annoying to debug and generally to handle: if we can avoid having people using the GlobLoader with *Aware fixtures, then let's not encourage bad practices and let them define the services.

@schmittjoh assuming the GlobLoader builds (internally) a ClassLoader, it becomes really tricky to use composition here

[schmittjoh]

For composition, it is enough to wrap the outer-most loader and iterate over the fixtures before returning them.

On Tue, Feb 12, 2013 at 10:53 AM, Marco Pivetta notifications@github.comwrote:

@stof https://github.com/stof I would totally encourage people wanting fixtures as services to do so instead of using initializers... They are always a bit annoying to debug and generally to handle: if we can avoid having people using the GlobLoader with *Aware fixtures, then let's not encourage bad practices and let them define the services.

@schmittjoh https://github.com/schmittjoh assuming the GlobLoaderbuilds (internally) a ClassLoader, it becomes really tricky to use composition here

— Reply to this email directly or view it on GitHubhttps://github.com/doctrine/data-fixtures/issues/89#issuecomment-13425239.

[stof]

@Ocramius implementing ContainerAwareInterface to have access to the container is far easier for users than having to define their fixtures as a service. Thus, it would be incompatible with the autodetection of fixtures through the GlobLoader. So IMO, we should keep the current feature of DoctrineFixturesBundle (which does not forbid us to also implement fixtures as service for people wanting them)

[Ocramius]

@stof yeah, I'm just arguing that this just encourages people to have the very typical:

class Foo implements BarAwareInterface { /**/ } $a = new Foo();

And then opening an issue somewhere because they don't get the fact that such an instance needs to go through an initializer to have bar. I'll cut it here because it's OT, but I will never get to like *Aware.

@schmittjoh is right on this one: just wrap load so that the retrieved values are iterated and passed to the initializer

@guilhermeblanco can you fix the typehints from array to SomeType[] where possible?

[juriansluiman]

I have two comments:

1. What are the dependencies (version)? It's good to know to what version you stick for Common/ORM etc, so people know if they can use the new fixtures within their existing apps.
2. Please, please focus on multiple paths for fixture data. It was a real pain in the ass with the old setup, we hacked around to make it fit our modularity principles. It will be the case for example in any large php framework (Symfony, ZF2) where modules (or bundles) would provide their own data and you simply run one command to load all data from all modules, or specify which modules you want to load the fixtures for.

I notice the ChainLoader, but I would really advice to take above use case as an example and make sure (eg also from the command line, the configuration etc) this will be possible. Or @Ocramius we enable the DoctrineModule to be configure the module's fixture paths with the chain loader and provide our own ZF2 CLI route for it.

[stof]

@juriansluiman I don't see anything in this new design requiring to change the requirement (Common 2.2+)

and the symfony bundle allows loading fixtures from several paths

[Ocramius]

@juriansluiman integrating this with the modules is a secondary issue right now IMO. DoctrineModule doesn't support this OOTB right now: still waiting to integrate https://github.com/Hounddog/DoctrineDataFixtureModule /cc @Hounddog

[guilhermeblanco]

@ocramius

As discussed on IRC, there's still this weird parallelism between connection and manager registries... Is there some way of getting rid of it?

No. They're different interfaces and they serve different purposes. When dealing with a DBALExecutor, you may or may not have the ObjectManager, depending of the user's configuration. So as part of the injection, I can only rely on the ConnectionRegistry, but sometimes you may receive a ManagerRegistry, because the latter extends the former.

Also: is the reason why the various executors and purgers have access to the registries? If it's for lazy loading, then it's not their responsibility IMO :)

A Purger is created from Executor. This means Executor is the top-most accessor of Registries. A Purger is then lazy-loaded inside of Executor, and requires the correspondent Registry to be able to execute its function. An alternative would be to rename Executor to Persister, and create a new Executor which holds the Persister and Purger, then making it Registry independent. I thought about it, but I don't see a value to add this extra layer in our code.

@stof

@guilhermeblanco in such a setup, where would you handle the ContainerAwareInterface of Symfony. Currently, it is done in the loader but it does not fit well with your new design with several loaders.

I received some comments straight to my email and one in particular makes perfect sense, which could also apply here. On version 1.X, we have some events being triggered at selected locations. Right now in this architecture, there's no concept of event triggering. I could also inject an EventManager at Importer and trigger a preLoad and postLoad. By doing that, first the name becomes wrong, it would be renamed to import (preImport, postImport) in Fixture. I think I'll just do it. Now, by having these events, you could simply write your ContainerAwareInterface (yuck, "Interface" suffix =P) as a preImport listener. Another alternative would be a protected method called assignDependencies($fixture) during the execution loop that injects not only the ReferenceRepository, but also whatever else you want by extending the Importer class. This will be also valid for projects like FLOW3 which injects all dependencies using annotations too.

@schmittjoh

@guilhermeblanco, do you have some before/after analysis, or the main goals of the refactoring?

The main purpose is to bring up to date with our current solution. When we released data-fixtures, we never considered people using exclusively DBAL and not the ORM, so this is a design limitation (look at the Fixture interface on current code). We're trying to address not only this, but also other limitations for loading files, etc.

@ocramius

@guilhermeblanco can you fix the typehints from array to SomeType[] where possible?

Hm... I don't think the tool allows me, but I'll try. =)

[guilhermeblanco]

Also, injecting also an EventManager will require a change in Importer. Right now it takes 2 constructor arguments and also has 3 dependent instances, which increases the coupling of this component with others, making it harder to test. This also breaks a rule of Object Calisthenics, that we try to follow.

I'll create a separate class called Configuration that includes the dependencies (EventManager, ExecutorFactory, CalculatorFactory and ReferenceRepository), include an ability to add new Calculators and consume this class inside of Importer.

Makes sense guys?

[stof]

A preImport event would work fine for the use case I described. I looks good and gives flexibility to hook whatever we want.

[guilhermeblanco]

Following up with our plan, I updated the diagrams with the following changes:

• Renamed Fixture::load to Fixture::import
• Add missing TopologicalSorter class and pointed out its dependencies
• Decoupled dependencies of Importer into a Configuration class
• Add EventManager to the game for event triggering (so far, preImport, postImport)
• Modified ExecutorFactory::getExecutor to receive name instead of a Fixture instance

Pending for resolution:

• Should we turn every Fixture step into a listener? This means Executor execution would be a listener, as well as Reference injection. You'd recommend that for better extensibility, but it would require that our EventManager support prioritization of listeners (which it doesn't atm). Also, this would make our code even more decoupled, but will probably have a performance hit. We did the same design decision on ORM, but personally here it wouldn't impact too much. That way, we would rely on interfaces to inject dependencies and it would also be a great proof of concept that our event system in this specific tool works. That would also help 3rd party tools to implement their own desired features (Symfony2, FLOW3, etc).
• How would we inject the related Registry to the Fixture? If we embrace the first resolution pending item, we'd create some interfaces for each Executor, allowing us to remove the getExecutorName method and the injection being done through the event listener.

@asm89 @beberlei @Ocramius @jwage @FabioBatSilva @jmikola @stof @schmittjoh @Seldaek @fabpot Another round of evaluation and ideas for our pending resolutions?

Here are the updated diagrams:

Doctrine\Fixture

Doctrine\Fixture\Executor

Doctrine\Fixture\Loader

Doctrine\Fixture\Purger

Doctrine\Fixture\Reference

Doctrine\Fixture\Sorter\Calculator

Doctrine\Fixture\Sorter\Type

[guilhermeblanco]

Missing graph: Package dependencies

[beberlei]

I am strongly against the event listener idea. it makes the whole thing complicated and the API blurry.

Wasn't the plan to inject the services visitor pattern? If i have an ORM Fixture, i extend from an orm fixture base class. There is a visitor method on it delegating back to the executor or some kind of factory, which then uses a method provided on the ORM fixture base class to inject the ORM. Same for connection, mongodb, etc..

Otherwise when going for constructor injection, we need some sort of abstract factory pattern that is somewhere in the loader component or directly connected to that.

[stof]

@beberlei but then, how do you hook the container-aware stuff in the system to keep the existing feature (without extending all loaders of course) ?

[schmittjoh]

Again, this can be easily done via composition:

class DicLoader implements Loader
{
    private $delegate;

    public function load()
    {
        $fixtures = $this->delegate->load();
        foreach ($fixtures as $fixture) {
            if ($fixture instanceof Container) { /* inject */ }
        }

        return $fixtures;
    }
}

This just needs to wrap the outer-most loader, for example the ChainLoader. Otherwise, I agree that we should try to avoid events and instead provide explicit extension points.

[javiereguiluz]

I'm sorry to interrupt this technical conversation with a not so technical comment, but I think that this discussion is wrong. You are talking about how to do something without having discussed first what to do.

Can we see a real example of the new proposed data fixtures classes? Can we discuss about how to make Doctrine Fixtures easy to create and use? Can we talk about how to streamline the experience of using Doctrine Fixtures both with Symfony and in standalone PHP projects?

In the end, most of the programmers that will use these fixtures won't care about registries, compositions, executors and class diagrams. My guess is that most programmers will care about:

• How can I easily create the fixtures of my application?
• How can I easily set the explicit order in which they must be loaded?
• How can I easily integrate data generating libraries such as Faker?
• How can I easily set cross references between fixtures files?
• How can I easily access to the database from inside a fixtures class?
• How can I easily distinguish between the real fixtures required for the proper functioning of the application and the fake fixtures used in tests?
• How can I easily create fixtures for each Symfony2 execution environment?

Lastly, it would be nice to plan in advance the error situations a user may incur when using the new Doctrine Fixtures and prepare some explanatory error messages.

[stof]

For cross-references, read the readme as the ReferenceRepository system already exists. For the order, see the Dependent and Ordered interfaces you can implement on fixtures (already available in 1.x as DependentFixtures and OrderedFixtures)

[lcobucci]

Any news about this? The 2.0 branch is stopped a long time ago, and I believe that the 1.x implementation really requires a major refactor...

[guilhermeblanco]

@lcobucci Branch 2.0 is mainly done. It misses documentation only. Once this is ready, we need to coordinate with other projects (such as sf2 and zf2 integrations) to update their composers too.

Besides that, sf2 bundle is in the oven missing a few tweaks to be complete.

[lcobucci]

Alright, on #115 I've documented the basic usage of the new version and a basic console command to help the integration with other projects.

[stof]

@guilhermeblanco I found an issue in the 2.0 Reference system: the objects returned by the ReferenceRepository are not managed by the manager, which means that the user has to merge them before being able to use them. In the 1.0 implementation, the repository was in charge of handling this, making it transparent for the user

[guilhermeblanco]

@stof will update the code to address it. =)

[tPl0ch]

@guilhermeblanco I hope you will merge the Purgers soon :-)

[derrabus]

We're currently planning 2.0 as a cleanup release with stronger native typing. I'm going to close this > 10 years old issue as not planned in order to avoid confusing with the actual 2.0 release.