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

Foreign key configuration for BelongsTo or HasMany relationship. #998

Closed
scaabel opened this issue Nov 10, 2022 · 3 comments
Closed

Foreign key configuration for BelongsTo or HasMany relationship. #998

scaabel opened this issue Nov 10, 2022 · 3 comments
Assignees
Labels
feature New feature or request

Comments

@scaabel
Copy link

scaabel commented Nov 10, 2022

Description

To have a centralized foreign key configuration for relationships. In the tenancy config, we can add a key to specify the value of the foreign key to reference the tenant's column.

// tenancy.php

'foreign_keys'  => [
    'tenant' => 'tenant_id'
]

Why this should be added

I'm currently using uuid as the primary key for tenants and based on the documentation to override the default tenant_id foreign key, we need to change BelongsToTenant::$tenantIdColumn. I noticed that the HasDomains trait is specifically defining the foreign key as tenant_id.

// Concerns/HasDomains
/**
 * @property-read Domain[]|\Illuminate\Database\Eloquent\Collection $domains
 */
trait HasDomains
{
    public function domains()
    {
        return $this->hasMany(config('tenancy.domain_model'), 'tenant_id') <---- specifying tenant_id ;
    }

// ...rest of code
}

If we were to implement a configurable foreign key in the tenancy config file. We can perhaps do something like.

// Concerns/HasDomains
/**
 * @property-read Domain[]|\Illuminate\Database\Eloquent\Collection $domains
 */
trait HasDomains
{
    public function domains()
    {
        return $this->hasMany(config('tenancy.domain_model'), config('tenancy.foreign_keys.tenant');
    }

// ...rest of code
}

// Concerns/BelongsToTenant
/**
 * @property-read Tenant $tenant
 */
trait BelongsToTenant
{
    public function tenant()
    {
        return $this->belongsTo(config('tenancy.tenant_model'), config('tenancy.foreign_keys.tenant'));
    }

    public static function bootBelongsToTenant()
    {
        static::addGlobalScope(new TenantScope);

        static::creating(function ($model) {
            if (! $model->getAttribute(config('tenancy.foreign_keys.tenant')) && ! $model->relationLoaded('tenant')) {
                if (tenancy()->initialized) {
                    $model->setAttribute(config('tenancy.foreign_keys.tenant'), tenant()->getTenantKey());
                    $model->setRelation('tenant', tenant());
                }
            }
        });
    }
}
@scaabel scaabel added the feature New feature or request label Nov 10, 2022
@stancl
Copy link
Member

stancl commented Nov 10, 2022

I see, seems like the HasDomains trait should be using something like BelongsToTenant::$tenantIdColumn as well instead of the literal tenant_id string.

We changed this in v4 to use a config key instead of a trait property, since PHP 8.1 shows deprecation notices when you try to access trait properties (#854): 24146b2

But I see your point that this relates to HasDomains as well, and not just single-db Tenancy traits/columns.

The change you proposed to BelongsToTenant seems to be fully implemented here already (correct me if I'm wrong): 24146b2#diff-3f95188061d3fafb597105f47adbb923c98d5b4da9a9839c4a944627c5277981R17-R27

And for the HasDomains trait, you can currently just override the method, but I agree that we should add a config key.

@stancl
Copy link
Member

stancl commented Nov 10, 2022

Thanks for opening the issue!

@stancl stancl closed this as completed Nov 10, 2022
@scaabel
Copy link
Author

scaabel commented Nov 10, 2022

I see, seems like the HasDomains trait should be using something like BelongsToTenant::$tenantIdColumn as well instead of the literal tenant_id string.

We changed this in v4 to use a config key instead of a trait property, since PHP 8.1 shows deprecation notices when you try to access trait properties (#854): 24146b2

But I see your point that this relates to HasDomains as well, and not just single-db Tenancy traits/columns.

The change you proposed to BelongsToTenant seems to be fully implemented here already (correct me if I'm wrong): 24146b2#diff-3f95188061d3fafb597105f47adbb923c98d5b4da9a9839c4a944627c5277981R17-R27

And for the HasDomains trait, you can currently just override the method, but I agree that we should add a config key.

Thanks @stancl .

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

No branches or pull requests

2 participants