Skip to content
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

[5.4] Factory: refresh model(s) from database when created. #19621

Closed

Conversation

mathieutu
Copy link
Contributor

Hi,
Here we are for the third PR of the day!
When we create some models from FactoryBuilder, it saves it in database, but the model is not refreshed from it.

It can create some problem, because the object you have is not the exact reflect of the true model stored in database. I think specially at all the default values in database.

Example:

Before the PR:

 dd(factory(Param::class)->create()->toArray());

array:10 [
  "section_id" => 1
  "title" => "textarea_jh8"
  "ref" => "textarea_M4L"
  "max_width" => 6
  "max_height" => 10
  "type" => 1
  "order" => 1
  "updated_at" => "2017-06-15 16:17:02"
  "created_at" => "2017-06-15 16:17:02"
  "id" => 246
]

After the PR :

 dd(factory(Param::class)->create()->toArray());

array:13 [
  "id" => 247
  "section_id" => 1
  "order" => 1
  "title" => "textarea_T8N"
  "placeholder" => null // <--
  "ref" => "textarea_ljF"
  "max_width" => "1"
  "max_height" => "5"
  "is_croppable" => false // <--
  "type" => 1
  "required" => false // <--
  "created_at" => "2017-06-15 16:17:50"
  "updated_at" => "2017-06-15 16:17:50"
]

I unfortunately didn't test it because I didn't find any test about FactoryBuilder in the framework.. 😰
I don't have the time right now to write them all, maybe another day..!

This PR is made above the previous one (#19616) because it uses the fresh() method on Eloquent Collection. I split them in two PR because they are really two distinct functionalities. Only the last commit is for this one, so it's better to merge the first one (#19616) before this one.

See you for the next one guys!
(yeah it's almost ready in local, maybe tomorrow...)
Matt'

@devcircus
Copy link
Contributor

Seems like that is the nature of 'create' in general. You get back what you've provided. Otherwise you're making another trip to the dB when many times all you want to do is create something and move on.

@mathieutu
Copy link
Contributor Author

Hum... I understand your pov, and it can be ok for the Model::create() method..
Otherwise for me when you use a factory builder, it's often to have quickly a fully operational model, and especially for testing. So for me you directly need a real one.

I had some problem with assertion where I was comparing collections from factories, and collections from builder, and the difference was all the fields not set by the factories, but by the database (null and default). It's something we don't think about, and it can help a lot.

It cost nothing to fresh it, but it can bring back a lot imo.

If you want we can just add a new method which create and fresh..?

@deleugpn
Copy link
Contributor

Maybe you can just do factory(Param::class)->create()->fresh()?

@mathieutu
Copy link
Contributor Author

Yeah, it's what I had to do on every factory in my tests.
It's why I thought about this PR.
As I said it costs (almost) nothing, but it will help a lot.

Here, a real example about that:

    public function testToVue()
    {
        $section = factory(Section::class)->create()->fresh();

        $paramsShown = collect()->times(2, function ($time) use ($section) {
            return factory(Param::class)->create([
                'section_id' => $section->id,
                'type' => $time,
                'order'    => $time,
            ])->fresh();
        });

        factory(Param::class)->create([
            'section_id' => $section->id,
            'type' => Param::TYPE_HIDDEN,
        ])->fresh();

        $this->assertEquals(collect([
            'title'      => $section->title,
            'column'  => $section->column,
            'params'    => $paramsShown->map->toVue(),
        ]), $section->toVue());
    }

If I forget the fresh() methods here, the assertEquals will fail, because the toVue() methods load from database. So we have to think about putting it each time we create a factory. And it's the same for all the tests, of all the models...

@tillkruss
Copy link
Contributor

Can't you solve this by simply extending/completing your factories?

@mathieutu
Copy link
Contributor Author

@tillkruss Yeah as I said I totally can manage that on my own projects, but I think it interesting enough to share it, and make it as a default.

In my opinion a factory is the easy and quick way to have a full working model. Not just a part of it. It's why I'm proposing to do it directly.

@taylorotwell
Copy link
Member

I think for now I'll just let people define default values in their factories rather than hit the DB each time again.

@calebporzio
Copy link
Contributor

@mathieutu - for what its worth, I ->create()->fresh() everywhere and would love this functionality baked in.

@mathieutu
Copy link
Contributor Author

Ok no problem, thank you all for your comments!

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.

6 participants