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

Introduces UseFactory attribute #54065

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

christopherarter
Copy link
Contributor

Description

This PR introduces a new UseFactory attribute that provides a more elegant way to associate model factories with Eloquent models using PHP 8.0+ attributes. This is especially useful for factory registration for models that are outside of the typical App\Models namespace and improves code readability (in my opinion).

Usage

<?php

namespace App\SomeFeatureDomain\Models\Comment;

use Database\Factories\CommentFactory;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Illuminate\Database\Eloquent\Attributes\UseFactory;

#[UseFactory(CommentFactory::class)]
class Comment extends Model
{
    use HasFactory;
    // ...
}

Instead of defining a new method for newFactory(), we can simply allow developers to add the Attribute.

Automatic inverse detection

When using this attribute, the factory for the given model will automatically be set, so there is no need to define a protected $model = \App\SomeFeatureDomain\Models\Comment::class from our example above.

Completely Optional & Fully backwards compatible

This logic only runs on classes that implement this attribute. Developers are still free to define static newFactory methods as they have before.

Priority Order

The factory resolution follows this priority:

  1. Static $factory property if defined
  2. UseFactory attribute if present
  3. Convention-based factory resolution

Requirements

  • PHP 8.0+ (required for attributes support)

Testing

This PR includes tests that verify:

  • Proper factory instantiation via attribute
  • Automatic model name resolution

@taylorotwell
Copy link
Member

I think I'm on board overall. Any reason you didn't just choose Factory as the attribute name? 👀

@christopherarter
Copy link
Contributor Author

@taylorotwell No particular reason other than for an explicit name to tell the developer exactly what it does, and to follow a more action-focused naming convention of the other attributes, e.g. ScopedBy, ObservedBy. But, it's a very loosely held opinion.

Also, probably just React leaking into my brain 😅

@shaedrich
Copy link
Contributor

shaedrich commented Jan 3, 2025

One might name it FakeWith() or the like but neither do I think, "factory" can be made into a useful participle (FactoriedBy wouldn't be proper English and FactorizedBy sounds too complicated for what it does) nor will the alternative (e.g. FakeWith/FakedBy) be 100% intuitive and fitting.

@christopherarter
Copy link
Contributor Author

@taylorotwell Would you like me to change to Factory instead? No biggie.

@taylorotwell
Copy link
Member

taylorotwell commented Jan 6, 2025

Hmm, got it. Good point on ScopedBy, etc. I would probably name this HasFactory then @christopherarter ... errr nvm that probably isn't possible due to already having a HasFactory trait.

@shaedrich
Copy link
Contributor

shaedrich commented Jan 6, 2025

Hmm, got it. Good point on ScopedBy, etc. I would probably name this HasFactory then @christopherarter ... errr nvm that probably isn't possible due to already having a HasFactory trait.

Well, it's possible—it could just end up being a PITA to keep in mind to alias one of them 😂

@christopherarter
Copy link
Contributor Author

@taylorotwell Yeah that was my conclusion as well. Which, is how I arrived at UseFactory, since it's descriptive & phrased as an action to follow that convention, and will likely seem familiar or intuitive from React pervading throughout the dev ecosystem.

Naming is hard!

@Rizky92
Copy link

Rizky92 commented Jan 10, 2025

I was thinking it could be named as CreateFactoryUsing since some parts use this naming terse within Laravel, albeit in methods.

<?php

namespace App\SomeFeatureDomain\Models\Comment;

use Database\Factories\CommentFactory;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Illuminate\Database\Eloquent\Attributes\CreateFactoryUsing;

#[CreateFactoryUsing(CommentFactory::class)]
class Comment extends Model
{
    use HasFactory;
    // ...
}

@christopherarter
Copy link
Contributor Author

@Rizky92 another good suggestion!

Just waiting to see where we land from the maintainers, and I'll be happy to update the name to whatever we land on. This will be my first contribution to core so, hoping it gets in 🤞🥹

@laserhybiz
Copy link

How about getting rid of the trait call as well? why should we need to define both the trait and the attribute

@taylorotwell taylorotwell merged commit ffa1a12 into laravel:11.x Jan 15, 2025
37 of 38 checks passed
@raz-iacob
Copy link

I noticed that I still need to add
/** @use HasFactory<\Database\Factories\CommentFactory> */
for phpstan. Is this a larastan issue?

@stsepelin
Copy link

Hello,

I also noticed that PhpStorm is complaining that the factory class couldn't be found if getting rid of protected $model = \App\SomeFeatureDomain\Models\Comment::class in the factory class.

Screenshot 2025-01-24 at 15 02 05

@christopherarter
Copy link
Contributor Author

@raz-iacob @stsepelin Noted, I'll try and reproduce and fix stan 👍

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.

7 participants