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

Fix Database Factory for Winter use #201

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Fix Database Factory for Winter use #201

wants to merge 4 commits into from

Conversation

mjauvin
Copy link
Member

@mjauvin mjauvin commented Jan 10, 2025

No description provided.

@mjauvin
Copy link
Member Author

mjauvin commented Jan 10, 2025

@sephyld how did you manage to use this before this PR?

@mjauvin
Copy link
Member Author

mjauvin commented Jan 10, 2025

@LukeTowers should I move the HasFactory trait under Database/Traits or Database\Factories\Traits or other ?

use Illuminate\Support\Str;

/**
* @template TModel of \Illuminate\Database\Eloquent\Model
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this used for? Should it be pointing at the winter base model class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, I copied this from Laravel.

I tried removing it completely and things still work as expected. It's probably only used by the docblock generator.

/**
* Get a new factory instance for the model.
*/
public static function factory(callable|array|int|null $count = null, callable|array $state = []): BaseFactory
Copy link
Member

@LukeTowers LukeTowers Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be returning a Winter BaseFactory, instead of a Laravel one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if we also change this in Winter repo's create:factory stub.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here: modules/system/console/scaffold/factory/factory.stub

@sephyld
Copy link

sephyld commented Jan 12, 2025

@sephyld how did you manage to use this before this PR?

Hi @mjauvin.

If you are referring to the factory not managing to resolve the model's class name, I just specify the model class using the $model property.

<?php

namespace Winter\Demo\Database\Factories;

use Illuminate\Database\Eloquent\Factories\Factory;
use Winter\Demo\Models\ModelKlass;

/**
 * ModelKlassFactory Factory
 * @extends \Illuminate\Database\Eloquent\Factories\Factory<\Winter\Demo\Models\ModelKlass>
 */
class ModelKlassFactory extends Factory
{

    protected $model = ModelKlass::class;

    /**
     * Define the model's default state.
     *
     * @return array<string, mixed>
     */
    public function definition()
    {
        return [
            // factory definition
        ];
    }
}

In hindsight, I think I should have added it in the PR that took care of the create:factory command. Sorry. If you want, I can take care of it now

@mjauvin
Copy link
Member Author

mjauvin commented Jan 12, 2025

@sephyld how did you manage to use this before this PR?

If you are referring to the factory not managing to resolve the model's class name, I just specify the model class using the $model property.

Actually, I'm referring to the Model not resolving the Factory. i.e. Model::factory() doesn't work without this PR.

@sephyld
Copy link

sephyld commented Jan 13, 2025

@sephyld how did you manage to use this before this PR?

If you are referring to the factory not managing to resolve the model's class name, I just specify the model class using the $model property.

Actually, I'm referring to the Model not resolving the Factory. i.e. Model::factory() doesn't work without this PR.

I see. I didn't think about that, I never use the model class to resolve its factory. I only use the factory class directly

@LukeTowers
Copy link
Member

@sephyld if you're able to improve the magic in Winter at all that would be much appreciated ❤️

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

Successfully merging this pull request may close these issues.

3 participants