thephpleague / factory-muffin

Enables the rapid creation of objects for testing
https://factory-muffin.thephpleague.com/
MIT License
531 stars 72 forks source link

Support for DataMapper pattern #408

Closed DavertMik closed 8 years ago

DavertMik commented 8 years ago

In FactoryMuffin v2.x I could define my own saver in setCustomSaver method and have it working with DataMapper pattern. In 3.x I see there is no such option left. I need to create new class RepositoryStore or some kind of.

Is it possible to include such class by default in 3.0 or bring setCustomServer method back?

GrahamCampbell commented 8 years ago

In 3.x, the recommended practice is to extend the https://github.com/thephpleague/factory-muffin/blob/master/src/ModelStore.php class.

GrahamCampbell commented 8 years ago

Then, just provide an instance as the first param to https://github.com/thephpleague/factory-muffin/blob/master/src/FactoryMuffin.php#L63.

GrahamCampbell commented 8 years ago

This felt like a much cleaner way of doing it to me.

DavertMik commented 8 years ago

Why not to include RepositoryStore into the FactoryMuffin then? DataMapper is pretty popular thing, you know

DavertMik commented 8 years ago

I can send PR for it

GrahamCampbell commented 8 years ago

I'm not really against it if you have the time to make a PR. I don't have the time to add it myself atm though.

DavertMik commented 8 years ago

Ok. I will try to get it implemented today.

DavertMik commented 8 years ago

I have implementation for v2 so I don't see problems to make it work in v3

DavertMik commented 8 years ago

Hey, @GrahamCampbell I've found one inconsitency which makes hard to implement proper wrapper and tests for Doctrine:

factory|User returns and Id of a user, while Doctrine does not operate with Ids and uses the instances instead. So if you are used to have

'user_id' => 'factory|User'

for Doctrine we need

'user' => 'factory|User'

and yes, here goes an issue, in second case we still have id value. Can this be changed? My proposal is for the first case to change syntax to factory_id|User which seems more logical, as we dont put the User into a value but it's Id. And this should be noted somehow. While factory|User will return a created user.

I know that makes BC break, but before 3.0 is released I think it is possible to have one.

I thought of the idea of overriding GeneratorFactory and FactoryGenerator for this case, but that seemed too much for me. And yes, it's not clear why factory| returns an Id. Maybe it could be a more general version where you can specify which field to return after is an instance is created.

P.S. Please reopen this issue. I'm working on it

GrahamCampbell commented 8 years ago

I know that makes BC break, but before 3.0 is released I think it is possible to have one.

I wanted to avoid BC breaks after having an RC release. I'll make a decision on what to do later.

DavertMik commented 8 years ago

As an alternative, we can have different word:

'user_id' => 'factory|User'
'user' => 'entity|User'
DavertMik commented 8 years ago

@GrahamCampbell ping?

jadb commented 8 years ago

I've run into the same problems trying with https://github.com/cakephp/orm.

+1 for BC before release - otherwise, it looses some of its framework agnostic-ity, at least IMO.

DavertMik commented 8 years ago

@GrahamCampbell so no response for a while. Should I send PR with BC break?

GrahamCampbell commented 8 years ago

Is it possible without the break?

DavertMik commented 8 years ago

Sure, just need to choose proper naming. OK if bc is priority I will send PR today without break 30 січ. 2016 01:19, користувач "Graham Campbell" notifications@github.com написав:

Is it possible without the break?

— Reply to this email directly or view it on GitHub https://github.com/thephpleague/factory-muffin/issues/408#issuecomment-177013987 .

GrahamCampbell commented 8 years ago

Thank you. :)

DavertMik commented 8 years ago

Done in #415

alexweissman commented 7 years ago

In 3.x, the recommended practice is to extend the https://github.com/thephpleague/factory-muffin/blob/master/src/ModelStore.php class.

@GrahamCampbell any chance we can see this covered in the docs soon?

GrahamCampbell commented 7 years ago

There is some docs here:

image

The specific implementation for each is not included. If I were to write that, then I might as well actually write a doctrine adapter. Unfortunately, I don't have time to do this, or to test it.

alexweissman commented 7 years ago

Thanks! Any chance of adding a getStore method to the FactoryMuffin class? I sometimes might want to modify the behavior of the custom ModelStore dynamically - for example, using Laravel's setConnection() on the model instance when saving/updating.

GrahamCampbell commented 7 years ago

I'd rather not allow mutation of the object after construction. I'd rather the object be constructed with the store set on it. You can extend the store or pass your own class, provided it implements the needed interface.

alexweissman commented 7 years ago

You can extend the store or pass your own class

Yes of course, but the problem is that if I want to control the behavior of the store after constructing the FactoryMuffin object, I need some way to get at the store.