Skip to content

[5.6] Added states to model factory after callbacks - #23551

Merged
taylorotwell merged 1 commit into
laravel:5.6from
unstoppablecarl:model-factory-add-after-states
Mar 21, 2018
Merged

taylorotwell merged 1 commit into
laravel:5.6from
unstoppablecarl:model-factory-add-after-states

Conversation

@unstoppablecarl

Copy link
Copy Markdown
Contributor

This is an addition to this recently merged PR made by @Indemnity83.
#23495

It adds the ability to bind the after callbacks to specific model factory states using the existing api pattern of existing create/make methods.

I this is not completely ready for PR. I wanted to submit it quickly after the above PR was merged to keep the discussion going. It still needs tests. Tagging the people that discussed the above PR to get their input: @deleugpn @browner12 @shadoWalker89

@taylorotwell

Copy link
Copy Markdown
Member

Seems OK to me. Can you add tests?

@tillkruss tillkruss changed the title added states to model factory after callbacks [5.6] added states to model factory after callbacks Mar 15, 2018
@unstoppablecarl

Copy link
Copy Markdown
Contributor Author

Yes I can. I am traveling currently and will be back in a few days. I will finish it up then. I just wanted to get it out there quick while it was being discussed.

@GrahamCampbell GrahamCampbell changed the title [5.6] added states to model factory after callbacks [5.6] Added states to model factory after callbacks Mar 17, 2018
@browner12

Copy link
Copy Markdown
Contributor

I'm assuming the most recent push was a mistake, correct? lots of unrelated changes

@unstoppablecarl

Copy link
Copy Markdown
Contributor Author

ugh yes. I was merging from upstream why did it do that? and how do I fix it?

Now that #23495 has been merged, my proposed changes will have been breaking. I assume that is not OK and have updated the code accordingly although I am disappointed that the api is less consistent because of it.

@shadoWalker89

Copy link
Copy Markdown
Contributor

@unstoppablecarl I would make a hard reset on the feature branch, and then push to github using the --force flag

@unstoppablecarl
unstoppablecarl force-pushed the model-factory-add-after-states branch from 41dbc74 to 350ea24 Compare March 19, 2018 16:51
@unstoppablecarl

unstoppablecarl commented Mar 19, 2018

Copy link
Copy Markdown
Contributor Author

There are some existing areas in Factory that do not have test coverage. Should I add tests for them?

@browner12

Copy link
Copy Markdown
Contributor

keep this PR simple. if the tests are not related to this PR, save those change for another PR

@tillkruss

Copy link
Copy Markdown
Contributor

A docs PR for this would be great.

@tillkruss

Copy link
Copy Markdown
Contributor

What's the best way to get a Faker instance in callbacks?

@unstoppablecarl

Copy link
Copy Markdown
Contributor Author

It gives the faker instance in the second argument of the callback.

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.

5 participants