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

Inject Fixtures loaders #242

Closed
theofidry opened this issue Aug 18, 2015 · 6 comments
Closed

Inject Fixtures loaders #242

theofidry opened this issue Aug 18, 2015 · 6 comments

Comments

@theofidry
Copy link
Member

As of now, when calling Nelmio\Alice\Fixtures::loadFiles(), the loader is resolved with the following function:

private static function getLoader(array $options)
    {
        // Generate an array key based on the options, so that separate loaders
        // will be created when we want to load several fixtures that use different
        // custom providers.
        $loaderKey = self::generateLoaderKey($options);
        if (!isset(self::$loaders[$loaderKey])) {
            self::$loaders[$loaderKey] = new Loader($options['locale'], $options['providers'], $options['seed']);
        }

        return self::$loaders[$loaderKey];
    }

But what if we want to use a custom Loader? A brand new loader or for example a Nelmio\Alice\Fixtures\Loader which has been initialised with some parameters.

@tshelburne
Copy link
Collaborator

Fixtures, as of right now, is basically just the path of least resistance. It's not intended to be the flexible answer, it just works for the simplest possible situation. I personally never use it.

From my perspective, the Fixtures is little more than a wrapper around Loader, with some conveniences thrown into the constructor. As soon as any custom behavior is needed, I suggest just using Loader or writing your own extension of Loader.

Thoughts?

@theofidry
Copy link
Member Author

So to recap, Loader does the actual loading to return the objects to persist. Fixtures is a bootstrap to take care of the persistence. I took a deeper look at Fixtures and from what I see its complexity (I don't mean it's a very complex object) comes from:

  • the fact that the loader is not injected by instantiated in it
  • the actual loading in which the loader is called and the objects retrieved are persisted.

The issue is that the loader is instantiated and there's not much that can be done since we're in a static context, unless we pass the loader in the ::load() function signature.

Back to our cases: having a custom Fixtures to handle the loader makes sense, what I'm not happy with though is that I'm using the exact same function ::load() so it's a plain copy/paste.

So here's my proposal:

  1. We let things as they are and we just add a chapter in the doc regarding this use case
  2. We add another Fixtures extension, which keeps the persistence logic as in the current Fixtures, but which does not aim to be used in a static context allowing to inject our Loader, custom or not, in it. This new extension would not be actually used in the library, but allow people to use their own loaders without having to redeclare a new extension.

I would like 2. of course, it opens up the library without being much to maintain. But obviously, if you can prefer to let this to the initiative of the users and choose 1. If you're fine with 2, I can take care of the PR.

@tshelburne
Copy link
Collaborator

Rather than introducing new static classes and yet another interface to the library, I think better documentation is the right answer. Unless I'm missing something, the following should work in even the most complicated circumstance:

$loader = new Loader();
/* any custom additions the user needs to $loader */
$persister = new DoctrinePersister($om);

$persister->persist($loader->load($fixtures));

That doesn't strike me as too much to ask of someone needing specific control, and doesn't expand ways to use the library even more.

Fixtures is a convenience, and should remain so - it makes decisions for you in the simplest cases, and let's you start your project quickly. I don't think we should encourage it being anything more than that, or that we should try to make the "convenience layer" more configurable. Instead, I think we should just offer up some tools to make it easier to customize things on your own. Personally, I think the Fixtures interface being too open to configurability (via constructor and method arguments) is what causes the discomfort, rather than the other way around.

Would love to hear your thoughts.

@theofidry
Copy link
Member Author

Well, what does not really please me is that's kind of incomplete.

This library provides a fixtures loader, which basically generate objects from files and a persistance layer via the PersisterInterface and a doctrine bridge. The last thing is, it provides a class responsible for loading the fixtures files, take care of the persistence layer and the pre/post-processing.

The issue risen here is the last part: due to being used statically it cannot be reused.

What I would prefer is, having a "clean" Fixtures non static, which does the work properly with the loader injected. Then the static Fixtures would be refactored to use the non static one and avoid duplication. That way you have the convenient Fixtures helper but also another one that can be used in another context like Laravel or Symfony.

You said there is not much, I agree for now so a doc may be fine. But depending of the other issues risen, especially #257 that may change. So for now I would say let's keep this and see how it goes.

@theofidry
Copy link
Member Author

After the discussion in #245, kind of thinking this should be only a short term thing as you're willing to get the persistence layer out. So some do should do (unless #257 call for it).

@theofidry theofidry added this to the 2.x milestone May 4, 2016
@theofidry
Copy link
Member Author

In the end is very unlikely, will be changed in v3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants