Skip to content
This repository

Data Fixtures version 2.0 #89

Open
guilhermeblanco opened this Issue February 11, 2013 · 36 comments

9 participants

Guilherme Blanco Marco Pivetta Christophe Coevoet Johannes Jurian Sluiman Benjamin Eberlei Javier Eguiluz Luís Otávio Cobucci Oblonczyk Thomas Ploch
Guilherme Blanco

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

Doctrine\Fixture\Executor
Doctrine_Fixture_Executor

Doctrine\Fixture\Loader
Doctrine_Fixture_Loader

Doctrine\Fixture\Purger
Doctrine_Fixture_Purger

Doctrine\Fixture\Reference
Doctrine_Fixture_Reference

Doctrine\Fixture\Sorter\Calculator
Doctrine_Fixture_Sorter_Calculator

Doctrine\Fixture\Sorter\Type
Doctrine_Fixture_Sorter_Type

Guilherme Blanco

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

Marco Pivetta
Collaborator

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!

Christophe Coevoet
Collaborator

@Ocramius the ManagerRegistry interface extends from ConnectionRegistry IIRC.

Christophe Coevoet
Collaborator

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.

Marco Pivetta
Collaborator

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

Christophe Coevoet
Collaborator

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

Marco Pivetta
Collaborator

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

Christophe Coevoet
Collaborator

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

Marco Pivetta
Collaborator

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

Christophe Coevoet
Collaborator

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

Marco Pivetta
Collaborator

@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
Johannes

Why not simply use composition for the DIC loader?

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

Christophe Coevoet
Collaborator

@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)

Marco Pivetta
Collaborator

@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

Johannes
Christophe Coevoet
Collaborator

@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)

Marco Pivetta
Collaborator

@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?

Jurian Sluiman

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.

Christophe Coevoet
Collaborator

@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

Marco Pivetta
Collaborator

@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

Guilherme Blanco

@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. =)

Guilherme Blanco

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?

Christophe Coevoet
Collaborator

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

Guilherme Blanco

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

Doctrine\Fixture\Executor
Doctrine_Fixture_Executor

Doctrine\Fixture\Loader
Doctrine_Fixture_Loader

Doctrine\Fixture\Purger
Doctrine_Fixture_Purger

Doctrine\Fixture\Reference
Doctrine_Fixture_Reference

Doctrine\Fixture\Sorter\Calculator
Doctrine_Fixture_Sorter_Calculator

Doctrine\Fixture\Sorter\Type
Doctrine_Fixture_Sorter_Type

Guilherme Blanco

Missing graph: Package dependencies
Package dependency

Benjamin Eberlei
Owner

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.

Christophe Coevoet
Collaborator

@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) ?

Johannes

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.

Javier Eguiluz

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.

Christophe Coevoet
Collaborator

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)

Luís Otávio Cobucci Oblonczyk

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

Guilherme Blanco

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

Luís Otávio Cobucci Oblonczyk

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.

Christophe Coevoet
Collaborator

@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

Guilherme Blanco

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

Thomas Ploch

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.