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

Laravel 8 factory support #861

Merged
merged 1 commit into from
Jul 7, 2021
Merged

Laravel 8 factory support #861

merged 1 commit into from
Jul 7, 2021

Conversation

canvural
Copy link
Collaborator

@canvural canvural commented Jul 2, 2021

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Closes #661

This PR adds support for Laravel 8 factories.

  • User::factory() would return UserFactory
  • User::factory()->createOne() would return User
  • Unfortunately there is no good solution to infer more precise return type for create method. It can either return collection of models, or single model. And this is decided by the count() method. This kind of fluent calls are hard to keep track with PHPStan.

I will add support for relations later (has{Relation} and for{Relation} method calls), I'm opening the PR now so if anyone wants to try over the weekend they could do so. I'd appreciate any feedback if anyone tries.

composer.json Outdated Show resolved Hide resolved
@spawnia
Copy link
Contributor

spawnia commented Jul 5, 2021

Thanks for getting this going, I tried it out in our project.

User::factory() would return UserFactory

Works beautifully.

User::factory()->createOne() would return User

Does not work for me yet.

@canvural
Copy link
Collaborator Author

canvural commented Jul 5, 2021

Thanks for getting this going, I tried it out in our project.

User::factory() would return UserFactory

Works beautifully.

User::factory()->createOne() would return User

Does not work for me yet.

Yeah, I forgot to write it in the description. Now that the Factory class is generic you should add @extends Factory<User> to your UserFactory class docblock.

I'll write this in release notes/changelog or upgrade guide before merging.

@spawnia
Copy link
Contributor

spawnia commented Jul 5, 2021

Now that the Factory class is generic you should add @extends Factory<User> to your UserFactory class docblock.

Sounds reasonable. Is there a way to infer this type when coming from a MyModel::factory() call? The information is there, can it be passed on to PHPStan?

@canvural
Copy link
Collaborator Author

canvural commented Jul 5, 2021

Now that the Factory class is generic you should add @extends Factory to your UserFactory class docblock.

Sounds reasonable. Is there a way to infer this type when coming from a MyModel::factory() call? The information is there, can it be passed on to PHPStan?

No. When you extend a generic class you only have 2 options. Either use @extends or make the class also generic.

@szepeviktor
Copy link
Collaborator

Is there a way to run PHP 8 tests only on PHP 8?

@spawnia
Copy link
Contributor

spawnia commented Jul 5, 2021

It works fine when using @extends, thank you.

I thought it would have to work, given from what I have experienced with inferred types of Collection. I do see the problem now, when making custom collection subclasses they have to use @extends too, right?

I am not sure I agree with PHPStan here, if that is where the limitation is coming from. Creating a subclass of a generic class does not necessarily erase its generic-ness.

@canvural
Copy link
Collaborator Author

canvural commented Jul 5, 2021

@szepeviktor Do you mean Laravel 8? Because we don't have any PHP 8 tests. For Laravel 8 yeah we are trying to run it on just Laravel 8, but seems it does not work exactly the way we wanted.

@spawnia It does not erase the genericness. If a class has a @template you have to pass something there when you are extending.

@szepeviktor
Copy link
Collaborator

Do you mean Laravel 8

Yes.

@spawnia
Copy link
Contributor

spawnia commented Jul 5, 2021

It does not eras the genericness. If a class has a @template you have pass something to there when you are extending.

My language was not that clear, and generic-ness is not a well-defined term. Having to provide an argument for the generic type parameter is what I deem erasure of the generic-ness. Any subclass extending from a generic class is no longer a generic class, unless the generic is redefined. I would think that when no type argument is provided, the generic keeps being generic.

@canvural canvural marked this pull request as ready for review July 5, 2021 16:55
@canvural
Copy link
Collaborator Author

canvural commented Jul 5, 2021

I think this is ready. I'll test it in some projects of mine. And maybe clean it a little more. Then we can 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.

Support Laravel 8 factories
4 participants