Remove FactoryMuffin from Models

[philsturgeon]

Copied from fork repo written by @scottrobertson.

@marcqualie brings up a good point that we should not really have the $factory definitions in our Models.

The way FactoryGirl does this is have the factory definitions separately.

For example, we could do:

<?php
FactoryMuffin::factory('User', array(
    'username' => 'string'
));

FactoryMuffin::factory('Message', array(
    'user' => 'factory|User'
));

This would keep the models clean, and remove the need to have testing specific code in production code.

[philsturgeon]

And then @FrenkyNet was all like...

@scottrobertson this is actually what I wanted to mention too. Having the definition in the model is smelly. I'd be more in favour a separation. It could be class based, where (like phpspec does) a namespace is prepended or a suffix appended so a classname can be mapped to it's factory config counterpart. Optionally an interface can be exposed which would allow for a per-class handling of the location/config/definition. Another option would be to create config files (returning arrays) in another directory where the files (like autoloading) can be mapped to configs.

[philsturgeon]

And @scottrobertson was saying

@FrenkyNet I like the idea of having it class based yeah. Do you have an idea of how you would want to do it? Using the namespace prepending/appending sounds good to me.

Having config files would be ok I guess, but it could get quite messy.

[frankdejonge]

To which @FrenkyNet replied:

In order to check whether a FQCN implements an interface you can check it like this:

if (in_array('MuffinStuffin\\DefinitionProviderInterface', class_implements($class, true))) {
    var_dump('fuck yeah');
}

In which case the interface could look something like:

interface DefinitionProviderInterface
{
    public static function getFactoryMuffinDefinition();
}

Which would return a Definition (either string FQCN or instance). A definition object would be created which in turn would supply the bootstrapper the needed information.

When this interface is not implemented a convention could be created. In this case I'm more leaning towards prefixing than suffixing to allow for nicer separation. Seperating the definition files in the FS as well as in code. These definitions, because it's used for testing, could then be made autoloadable by adding a autoload-dev clause (which only adds these classes to the auto-loaded when installing as --dev) eliminating clutter in production. So a class like League\Awesome\Sause Would then be converted to Muffin\League\Awesome\SauseDefinition.

The prefixed namespace and class based nature of the definitions enable definitions to be auto generated by a cli tool (future feature maybe?).

So, in short. Check FQCN for interface, if so, get it from the static method provided by the model. Otherwise autoload by convention. Wrapped by WhatEver\[FQCN]\Definition.

[scottrobertson]

@GrahamCampbell What are your thoughts on this?

[GrahamCampbell]

I definitely like the idea of separating the testing code from the model and I like the idea of having that interface.

[GrahamCampbell]

Any news on this?

[scottrobertson]

Not at all sorry, I have been really busy :(

[GrahamCampbell]

No problem. Should we shift the rc1 milestone one week in the future from it's current date then?

[scottrobertson]

I am not sure I am going to have time this week either. Stuff has come up that I need to do :( You could have a go at it? Or someone else could?

[GrahamCampbell]

Maybe you could just tag the rc today, and postpone this for 3.0?

[scottrobertson]

Yeah i think so. It's quite a big change and I think we have done a lot to justify v2 already

[ianterrell]

Would it be sufficient to expose the FactoryMuffin instance method define through a top level static function in the facade?

[scottrobertson]

@ianterrell Maybe yeah. I am trying to think of any consequences of storing $this->factories as a static variable instead.

[ianterrell]

@scottrobertson Thanks for thinking about this idea. If you find it useful, here's a merge request to expose it: #90

It looks from the unit tests that the define method is already used extensively.