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

Table in migration is not recognized if Schema facade starts specifying the connection #1058

Closed
lsalazarm99 opened this issue Dec 5, 2021 · 12 comments · Fixed by #1151
Closed
Labels
enhancement New feature or request

Comments

@lsalazarm99
Copy link

  • Larastan Version: 1.0.2
  • --level used: 9

Description

In the migration files, if we need to specify a connection for a specific schema operation, Laravel proposes Schema::connection('connection')->create('table', ...) (reference). If this is the case, the table is not being recognized by Larastan.

Laravel code where the issue was found

Schema::connection('connection')->create('table', ...)
@szepeviktor
Copy link
Collaborator

szepeviktor commented Dec 5, 2021

Hello Leonardo! 👋
Larastan recognizes simple migrations.
https://github.com/nunomaduro/larastan/blob/7d6e564144b2f9c231d0331d4083905a1b3e872b/src/Properties/SchemaAggregator.php#L62-L80

@szepeviktor szepeviktor added the enhancement New feature or request label Dec 5, 2021
@jamespavett
Copy link

Yea I've just realised I have the same issue. Would be nice feature to have.

@tyler36
Copy link

tyler36 commented Feb 28, 2022

Not sure if it's related but I'm getting a Access to an undefined property for 2 models; both of which have the following

    public function __construct(array $attributes = array())
    {
        parent::__construct($attributes);

        // Dynamically set default database 'connection' from config
        $this->setConnection(config('client-admin.database'));
        // Dynamically set default database 'table' from config
        $this->setTable(config('client-admin.table.clients'));
    }

The project is locked to Laravel 7 due to PHP 7.2 restrictions.

@szepeviktor
Copy link
Collaborator

Hello @tyler36 !
PHPStan does not recognize dynamic properties. I think having dynamic properties is a bad part of PHP.

@tyler36
Copy link

tyler36 commented Feb 28, 2022

@szepeviktor Thanks for the feedback. I have it set this way so I can dynamically change the database during testing.

@szepeviktor
Copy link
Collaborator

I see! So your class property is not dynamic but the table name is.

@tyler36
Copy link

tyler36 commented Feb 28, 2022

@szepeviktor yes. I've replicated an external database locally so I don't hit it during testing. Would this be considered a bug in how Larastan processes files? Do you want me to open a new issue?

@szepeviktor
Copy link
Collaborator

Would this be considered a bug in how Larastan processes files?

Larastan understands only simple migrations.
https://github.com/nunomaduro/larastan/blob/965a451948e3034693be7ca683c28b9db97e9711/tests/Application/database/migrations/2020_01_30_000000_create_users_table.php#L18-L31

@tyler36
Copy link

tyler36 commented Feb 28, 2022

Larastan understands only simple migrations.

I see. So Larastan does not support Schema::connection($this->connection) migrations. Thanks for clarifing.

@jamespavett
Copy link

Could Larastan not just ignore the connection definition when scanning migrations? Just if the underlying migration is valid, the connection property could just be disregarded.

@szepeviktor
Copy link
Collaborator

Yes it could.
PR-s are welcome!

@canvural
Copy link
Collaborator

Could Larastan not just ignore the connection definition when scanning migrations? Just if the underlying migration is valid, the connection property could just be disregarded.

Yes, this issue is just about that. That's why it's open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants