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

Revert getConnectionName change #99

Closed
wants to merge 2 commits into from
Closed

Conversation

eithed
Copy link

@eithed eithed commented Sep 2, 2021

This PR will:

  • remove usage of getConnectionName

I hope I'll be able to understandably present the issue which is caused by defining getConnectionName in Sushi traited models. I've added a test that showcases the issue, but it warrants an explanation why it's done this way as well.

To start from beginning - recently after updating Sushi to newest version I've noticed a fatal error that was caused by trying to access an Eloquent model through a relation with Sushi model. This is showcased in test and two models - in my system Quz is saved to database, but references Qux which exists in filesystem (e.g. Quz will be saved in the DB as ['title' => 'foo', 'qux_id' => 1])

From debugging (and my understanding) the connection that Laravel uses propagates through relationship, if it's defined on the models - ie. trying to access Qux::find(1)->quz will cause connection on Quz to be taken from Qux. As such, instead of using the default one (in my case psql), it will use SqlLiteConnection instead, as that's the connection that will be resolved using getConnectionName method.

The test showcases this - by default, default connection should be used (orchestra sets mysql as default one). This connection doesn't exist, so exception with Connection refused message should be raised. The erroneous behaviour of using sqlite file results in no such table: quzs exception message instead.

If getConnectionName method doesn't exist (or returns null) on Sushi trait, Laravel will correctly resolve default connection class. I don't know why this happens only if getConnectionName is defined on one of the related models, but not the other; when connectionName is set on Eloquent model, correct connection class is resolved, even when one is psql and the other is static::class.

As such the below works:

class Quz extends Model
{
    public function qux() : BelongsTo
    {
        return $this->belongsTo(Qux::class);
    }

    public function getConnectionName()
    {
        return $this->connection ?? config('database.default');
    }
}

This works as well:

class Quz extends Model
{
    protected $connection = 'psql';

    public function qux() : BelongsTo
    {
        return $this->belongsTo(Qux::class);
    }
}

Unfortunately this requires all Eloquent models that have a relationship with Sushi model to define connectionName explicitly - otherwise accessing given model through relationship will result in error.

To be fair, I'd prefer the current code to remain, as it elegantly resolves the exists validation. However, I feel that majority of users will never explicitly define connection name on their models and as such will experience this error. A solution might well be to warn users about the danger in the readme.

@eithed
Copy link
Author

eithed commented Sep 2, 2021

Actually the approach from #98 resolves this (and makes my test pass as well). I should look at the other PR before submitting 😄

@eithed eithed closed this Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant