Skip to content

[FIX] Missing state() function on Factory #198

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

Merged
merged 2 commits into from
Dec 14, 2016
Merged

[FIX] Missing state() function on Factory #198

merged 2 commits into from
Dec 14, 2016

Conversation

jakub-klapka
Copy link
Contributor

With laravel/framework 5.3.17, they have added states to model factories. Since function state() is called on every factory() (and entity()) calls, testing with Doctrines Factory will fail on undefined function.

This PR only supresses the error, but does not implement states functionality. It's up to you, if it should be merged.
Ideally, Laravel-Doctrine might adopt states functionality, since it is same, as defineAs functionality. It would be just more familiar to Laravel users. States have one advantage compared to defineAs - you can stack states up.

What approach do you think would be best for Laravel-Doctrine?

@patrickbrouwers
Copy link
Contributor

Hey @jakub-klapka, thanks for your PR.
Could you fix the CS please. :)

I don't know what state() does, do you have a link to the documentation about it? But if we can implement it, I'd say it's a good idea.

@jakub-klapka
Copy link
Contributor Author

States are described here: https://laravel.com/docs/5.3/database-testing#writing-factories
The main difference from yours defineAs is, that with states, you define only those properties, which are different from those in base factory. You can define multiple states. And when you are calling factory()->states(['one','two']), it would merge all state properties on top of base factory properties.

I think, that this is really neat, since you can combine those in many different ways. With defineAs, you don't have that much flexibility.

Thanks for considering this ;-)

@patrickbrouwers
Copy link
Contributor

Could you open an issue for this, so we can keep this (state support) in mind for the next release?

@patrickbrouwers patrickbrouwers merged commit e2a8780 into laravel-doctrine:1.2 Dec 14, 2016
@jakub-klapka
Copy link
Contributor Author

Sure, thank you for the merge.

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

Successfully merging this pull request may close these issues.

2 participants