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

[11.x] Feat: remove HasFactory in model when not required #53104

Merged
merged 7 commits into from
Oct 11, 2024

Conversation

MrPunyapal
Copy link
Contributor

A few days ago, I made a PR to improve the make:model command to include the factory generic PHPDoc when needed. Based on comments from @Jubeki and @jasonmccreary, it seems that it would be cool to remove HasFactory if we are not generating a factory at all.

examples:

Command

php artisan make:model Post

Output

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;

class Post extends Model
{
    //
}

Command

php artisan make:model Comment -f

Output

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;

class Comment extends Model
{
    /** @use HasFactory<\Database\Factories\CommentFactory> */
    use HasFactory;
}

@MrPunyapal MrPunyapal changed the title Feat: remove HasFactory in model when not required [11.x] Feat: remove HasFactory in model when not required Oct 10, 2024
@MrPunyapal
Copy link
Contributor Author

Something is wrong with failing tests.
Ps. I'm a windows user

@browner12
Copy link
Contributor

how common is it to not have a factory for a model? I don't think I've ever not made one.

@MrPunyapal
Copy link
Contributor Author

MrPunyapal commented Oct 10, 2024

It depends!

Not all developers do write tests.

Screenshot_20241011-003032.png

@browner12
Copy link
Contributor

if "not writing tests" is the reason behind it, maybe we don't want to make it easier on them. lol

@MrPunyapal
Copy link
Contributor Author

@browner12 Agreed!

Btw main reason behind this PR is these 2 comments!

#52855 (comment)
#52855 (comment)

@crynobone
Copy link
Member

I feel fine with these changes as you can still run UserFactory::new() instead of User::factory() all the time.

@crynobone crynobone marked this pull request as draft October 11, 2024 01:25
@crynobone
Copy link
Member

Convert to draft as tests are failing on Windows.

@@ -263,7 +263,7 @@ protected function buildFactoryReplacements()
$replacements['{{ factoryImport }}'] = 'use Illuminate\Database\Eloquent\Factories\HasFactory;';
} else {
$replacements['{{ factoryCode }}'] = '//';
$replacements["\n{{ factoryImport }}"] = '';
$replacements["{{ factoryImport }}\n"] = '';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code does not pass the tests on the local Windows machine. (but in workflow, it passed).

image

On Windows \r\n is used as a new line and in UNIX \n is used as a new line that's why I used PHP_EOL to specify it and it was passing in the Windows machine.

But as you can see in the failing test in workflow, it is \n instead of \r\n so I doubt if it is running on Windows.

image

Copy link
Member

Choose a reason for hiding this comment

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

can you test againts \r\n, \r and \n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see it passes with \r\n
image

@MrPunyapal MrPunyapal force-pushed the feat/has-factory-in-model branch 2 times, most recently from 268289c to 641b1d5 Compare October 11, 2024 07:03
@MrPunyapal
Copy link
Contributor Author

MrPunyapal commented Oct 11, 2024

@crynobone

I added replacements for both \n and \r\n this way it is passing at both places.

$replacements["{{ factoryImport }}\n"] = '';
$replacements["{{ factoryImport }}\r\n"] = '';

But ideally, it should pass with PHP_EOL so something is wrong with the workflow for sure.

$replacements['{{ factoryImport }}'.PHP_EOL] = '';

@MrPunyapal MrPunyapal marked this pull request as ready for review October 11, 2024 14:22
@taylorotwell taylorotwell merged commit 05b184c into laravel:11.x Oct 11, 2024
31 checks passed
@MrPunyapal MrPunyapal deleted the feat/has-factory-in-model branch October 11, 2024 14:59
timacdonald pushed a commit to timacdonald/framework that referenced this pull request Oct 15, 2024
)

* Refactor ModelMakeCommand to improve factory handling

* lint

* tweak

* tweak-2

* adding code which failing test in local on windows machine

* Refactor ModelMakeCommand to handle factory imports on Windows

* formatting

---------

Co-authored-by: Taylor Otwell <taylor@laravel.com>
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.

4 participants