The Wayback Machine - https://web.archive.org/web/20220701110442/https://github.com/laravel/framework/pull/35204
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

[8.x] Use dynamic app namespace in Eloquent Factory instead of App\ string #35204

Merged
merged 1 commit into from Nov 13, 2020

Conversation

0xb4lint
Copy link
Contributor

@0xb4lint 0xb4lint commented Nov 12, 2020

Currently, Eloquent Factory does not handle model namespaces other than the ones with App\ prefix.

(This is the only place in the framework where App\ string is used, all the other parts are using getNamespace() from Application.)

This PR eliminates the use of App\ namespace string and adds a dynamic way to determine the actual root namespace.
Test is included.

@driesvints driesvints changed the title Fix: use dynamic app namespace in Eloquent Factory instead of App\ string [8.x] Use dynamic app namespace in Eloquent Factory instead of App\ string Nov 13, 2020
@driesvints
Copy link
Member

@driesvints driesvints commented Nov 13, 2020

This has been attempted before btw and rejected. In general we don't recommend changing the App namespace.

@0xb4lint
Copy link
Contributor Author

@0xb4lint 0xb4lint commented Nov 13, 2020

I don't get why the Eloquent Factory is using hardcoded namepace and rest of the framework is handling it dynamically.
I'm using different namespace for ages and there was no issue at all except this one.

@@ -626,10 +627,13 @@ public function modelName()
{
$resolver = static::$modelNameResolver ?: function (self $factory) {
$factoryBasename = Str::replaceLast('Factory', '', class_basename($factory));
$rootNamespace = Container::getInstance()
->make(Application::class)
->getNamespace();
Copy link
Member

@taylorotwell taylorotwell Nov 13, 2020

Choose a reason for hiding this comment

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

What if this is called from outside of the Laravel framework?

Copy link
Contributor

@rodrigopedra rodrigopedra Nov 13, 2020

Choose a reason for hiding this comment

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

As in a Laravel app the Container is implemented by Application would this make sense?

$rootNamespace = method_exists(Container::getInstance(), 'getNamespace')
    ? Container::getInstance()->getNamespace()
    : 'App\\';

If it makes sense it could be extracted to a method as it would be needed in more places.

Copy link
Contributor

@rodrigopedra rodrigopedra Nov 13, 2020

Choose a reason for hiding this comment

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

Didn't see you commit 8bf80de

Guess that is a better solution

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.

None yet

4 participants