thephpleague / factory-muffin

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

Custom Maker is gone in v3 #466

Closed igorsantos07 closed 4 years ago

igorsantos07 commented 4 years ago

I got lost on the current website and found the feature I needed: customMaker (together with saver and deleter, of course).

Turns out the "new" v3 dropped the custom maker feature, or at least made it severely different. I guess I'll need to create a custom Store for the save and delete calls, but... there's no way to create a custom instantiator. The customMaker was moved to Definition, and the class itself is hardcoded, what means I'll have to write a custom maker for every definition I write? Or overwrite FactoryMuffin entirely, what's not exactly easy as well as I'd like to use it through Codeception.

igorsantos07 commented 4 years ago

In case this area gets changed, I noticed a mistake in the *Store::save() doc-block: it doesn't "keep track of it". Happens in all three store classes.

igorsantos07 commented 4 years ago

Not sure if this is a mistake, but shouldn't AbstractStore implement StoreInterface directly?

Or did I miss some OO classes in college? :joy:

GrahamCampbell commented 4 years ago

Not sure if this is a mistake, but shouldn't AbstractStore implement StoreInterface directly?

No, it doesn't have to. I've left it so that when you extend it, you decide if you want to implement the interface. Doesn't really matter tbh, and too late to change, since it'd be breaking.

Turns out the "new" v3 dropped the custom maker feature

It didn't get dropped. It got replaced with stores. You can either extend one of the stores, or implement a store wrapper that also implements the store interface, which is the compositional way to do things. The v2 way to do things was kinda hacky. People should not be afraid to define a class to do what they want. :)

igorsantos07 commented 4 years ago

Hmmm actually the missing interface does matter, as StoreInterface is type-hinted at FactoryMuffin. I might be being a bit shortsighted here, but what's the use for an extension of AbstractStore if not to feed into FactoryMuffin?

Yeah, classes don't bite, but from the outside, I don't see why split the customizable pieces. It might make sense internally, but to actual users, it's just more stuff to understand and write - if I can create, save and delete my models using the same class, why do I need to write two to get a Factory to do it?

I guess docs would help there, but it seems you find it better to leave people digging through the source code instead of documenting it while coding.

GrahamCampbell commented 4 years ago

but what's the use for an extension of AbstractStore if not to feed into FactoryMuffin?

Yes, I agree. If I were to do this again, I would have made the abstract store implement the interface, but it really doesn't matter. As you say, there is no reason not to add it, but we can't add it now because it'd be a pointless BC break.

Hmmm actually the missing interface does matter, as StoreInterface is type-hinted at FactoryMuffin.

But this means it does not matter. The typehint just means you really have to implement the interface. Extending the store is not enough, and indeed is optional.

I guess docs would help there, but it seems you find it better to leave people digging through the source code instead of documenting it while coding.

Yep. This should be included in the upgrading guide. Thus, I will close this issue. If you'd like to also provide a direct example in the main docs, I'd consider a PR. :)