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 ide-helper:models exception if model doesn't have factory #1196

Merged
merged 4 commits into from
Apr 1, 2021

Conversation

ahmed-aliraqi
Copy link
Contributor

@ahmed-aliraqi ahmed-aliraqi commented Mar 29, 2021

Summary

fixes #1188

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Code style has been fixed via composer fix-style

@ahmed-aliraqi ahmed-aliraqi changed the title Fix ide-helper:models exciption if model doesn't have factory Fix ide-helper:models exception if model doesn't have factory Mar 29, 2021
@Messhias
Copy link

I tested locally and this PR works fine, why isn't approved yet?

@barryvdh
Copy link
Owner

Sorry I can't respond to everything directly. There is an alternative PR open here: #1189

@barryvdh barryvdh requested a review from mfn March 30, 2021 15:18
@barryvdh
Copy link
Owner

@mfn Is this good to merge? Would rather fix this issue before tagging a new version.

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

@barryvdh yes, this LGTM and I prefer it technically over #1189 , approach make sense to me!

@mfn mfn merged commit 843067b into barryvdh:master Apr 1, 2021
@mfn mfn mentioned this pull request Apr 1, 2021
@ahmed-aliraqi
Copy link
Contributor Author

@mfn @barryvdh thanks for merging if you need any help just mention me ✌️

@wimski
Copy link
Contributor

wimski commented Apr 1, 2021

@ahmed-aliraqi @mfn @barryvdh

Shouldn't $modelName::newFactory() be $modelName::factory()? Because newFactory returns nothing by default, but factory does. And it will still use newFactory if it does return something.

https://github.com/laravel/framework/blob/8.x/src/Illuminate/Database/Eloquent/Factories/HasFactory.php

My models use the HasFactory trait and a corresponding factory exists for each model, but the annotation is not written to the model doc block.

@ahmed-aliraqi
Copy link
Contributor Author

@wimski By default $modelName::newFactory() return nothing it added to give ability to customize your own factory namespace different than the laravel's default

@wimski
Copy link
Contributor

wimski commented Apr 2, 2021

@ahmed-aliraqi

Yes, I get that the newFactory method enables you to customize your factory namespace. What I'm saying is that the factory method includes that support so there's no need to use the newFactory on its own.

$factory = static::newFactory() ?: Factory::factoryForModel(get_called_class());

Currently the IDE helper determines the factory class in two ways:

  1. As a templated string \Database\Factories\\{MODEL_NAME}Factory.
  2. Or by using the newFactory method to check for a custom namespace.

Both options do not work in my situation. I do use custom namespacing for both models and factories (sub division), but I don't use the newFactory method. The Factory::factoryForModel(get_called_class()) seems to figure out what the corresponding factory is, probably based on the $model property inside the factory.

For example:

  • App\Models\Users\User
  • Database\Factories\Users\UserFactory

My factories work fine using this setup without having to specifically declare my custom namespace in newFactory. The IDE helper won't write a factory doc block for my model though, because neither of the resolver rules match my use case. Switching from newFactory to factory would, while still supporting explicit custom namespaces.

@mfn
Copy link
Collaborator

mfn commented Apr 2, 2021

😱

What shall we do 😄

@wimski do you want to attempt another PR to continue discussion there maybe?

@ahmed-aliraqi
Copy link
Contributor Author

ahmed-aliraqi commented Apr 2, 2021

@wimski @mfn @barryvdh I suggest to move it to the config file to be able to customize your own namespace globally.

// config file:
[
  ...
  'factories_namespace' => null,
  ...
]
// ModelsCommand.php
$factory = config('ide-helper.factories_namespace') ?: "\Database\Factories\\{$modelBaseName}Factory";

If this solution is appropriate. I will create another PR.

@wimski
Copy link
Contributor

wimski commented Apr 2, 2021

@ahmed-aliraqi

I don't think that will solve anything. I'm working on my own PR to support both custom namespaces and namespaces that can be resolved by default.

@mfn

wimski do you want to attempt another PR to continue discussion there maybe?

#1201

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.

v2.9.1: artisan ide-helper:models stopped working after update
5 participants