8000 illuminate/database exception · Issue #36050 · laravel/framework · GitHub
[go: up one dir, main page]
Skip to content

illuminate/database exception #36050

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.

Sign up for GitHub

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

Jump to bottom
Closed
nbk-kolyan opened this issue Jan 26, 2021 · 11 comments
Closed

illuminate/database exception #36050

nbk-kolyan opened this issue Jan 26, 2021 · 11 comments

Comments

@nbk-kolyan
Copy link

Description:

Im not using laravel, using lumen. But this problem related to package we all use
illuminate/database version 8.24

Illuminate\Database\Eloquent\Factories\Factory
Use
use Illuminate\Contracts\Application;
instead of
Illuminate\Contracts\Foundation\Application;

this exception in try catch block - but i don't see reason not to using correct namespace

Steps To Reproduce:

File: \vendor\illuminate\database\Eloquent\Factories\Factory.php

/**
 * Get the application namespace for the application.
 *
 * @return string
 */
protected static function appNamespace()
{
    try {
        return Container::getInstance()
                        ->make(Application::class)
                        ->getNamespace();
    } catch (Throwable $e) {
        dd($e->getMessage());
        return 'App\\';
    }
}
@driesvints
Copy link
Member

Heya. I don't see Illuminate\Contracts\Application being used anywhere in both Lumen or Laravel.

@nbk-kolyan
Copy link
Author
nbk-kolyan commented Jan 29, 2021 •
edited
Loading

don't
@driesvints check this line - its wrong.

framework/src/Illuminate/Database/Eloquent/Factories/Factory.php

Line 10 in 6bc7d1b

use Illuminate\Foundation\Application;

Because in case of Lumen we don't use Illuminate\Foundation\Application
we have Laravel\Lumen\Application;
So in this case you should use contract

framework/src/Illuminate/Contracts/Foundation/Application.php

Line 7 in 6bc7d1b

interface Application extends Container

Use abstract not concrete )) In case packages are used in both frameworks (Lumen + Laravel)

@driesvints
Copy link
Member

If you need to use those factories please use Laravel instead.

@nbk-kolyan
Copy link
Author
nbk-kolyan commented Jan 29, 2021 •
edited
Loading

If you think that factories shouldn't be used in Lumen... Then it's up to you.
But from my point of view - if we can(and should) use illuminate packages in Lumen, then it should work correctly

@driesvints
Copy link
Member

Okay, I've just tried on a fresh Lumen install and factories are working fine for me. So I'm not sure if it's related to anything specific to your setup?

@vborduja
Copy link

The same trouble with lumen 8.2.1 and 8.2.2.
Class: Illuminate\Database\Eloquent\Factories\Factory
Line 10: use Illuminate\Foundation\Application;
8000

@driesvints
Copy link
Member

@vborduja maybe you should stop downvoting my comments and give me a proper way to reproduce this?

@nbk-kolyan
Copy link
Author

It's works )) Because it's in try - catch block. And it's returns App
But if, for some reasons we or any other users will decide to move app folder to another location, or change namespace
for example to AppTest
composer.json

    "autoload": {
        "psr-4": {
            "AppTest\\": "app/",
            "Database\\Factories\\": "database/factories/",
            "Database\\Seeders\\": "database/seeders/"
        }
    },

also change namespaces to new one, for making it works in bootstrap/app.php ( App -> AppTest)

framework/src/Illuminate/Database/Eloquent/Factories/Factory.php

Line 737 in 6bc7d1b

return 'App\\';

will return App\\
And there will 500 error
Class 'Database\Factories\AppTest\Models\UserFactory' not found
But if we will use Contracts in Factory
use Illuminate\Contracts\Foundation\Application;
then there will no be error.

Actually I think its just TYPO because everywhere in framework you use
Illuminate\Contracts\Foundation\Application (you can find in vendor folder)
And only at Factory.php uses
Illuminate\Foundation\Application

@driesvints
Copy link
Member

We don't support moving the app directory and we don't recommend changing the default namespace, sorry.

@alexmakii
Copy link
Contributor

@driesvints You are right, there is no direct harm as App\ namespace is default for laravel|lumen applications.

In our case, actual issue was the terrible performance degradation of our tests due to bad setup of xdebug in laradock on windows (wsl2).
In short:

  1. Every time Model::factory() is used - the Factory::appNamespace() is called in background.
  2. Factory::appNamespace() requests Illuminate\Foundation\Application from the container
  3. As lumen app is linked to laravel contract Illuminate\Contracts\Foundation\Application, container was throwing the BindingResolutionException.
  4. Due to the laradock performance issues - xdebug was spending ~4 secs on processing every exception being thrown.
  5. As final result the initial database seeding for our tests was taking up to 2 hours.

We fixed our problem by tweaking laradock setup, but during investigation was found that Factory accidentally uses direct application FQCN while illuminate contract should be used.

I've created a PR with the fix #36089

@driesvints
Copy link
Member

To be perfectly clear: I'm not opposed to the change to the contract itself. I'm just saying that we don't recommend changing the app directory or the namespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@driesvints @alexmakii @nbk-kolyan @vborduja
0