Changing primary key doesn't behave normally

[ElMatella]

Hi, I needed to change primary key for a model:

class Category extends Model 
{
	use Translatable;

	protected $primaryKey = 'hash';
}

However, when setting primary key like this, when calling the method getRelationKey(), it also returns hash:

public function getRelationKey()
{
    if ($this->translationForeignKey) {
        $key = $this->translationForeignKey;
    } elseif ($this->primaryKey !== 'id') {
        $key = $this->primaryKey;
    } else {
        $key = $this->getForeignKey();
    }

    return $key;
}

I don't quite understand how the elseif ($this->primaryKey !== 'id') works here and why is it implemented like that? I would have thought the following:

public function getRelationKey()
{
    if ($this->translationForeignKey) {
        $key = $this->translationForeignKey;
    } elseif ($this->primaryKey !== 'id') {
        $key = Str::snake(class_basename($this)).'_' . $this->primaryKey;
    } else {
        $key = $this->getForeignKey();
    }

    return $key;
}

I know I can make use of $translationForeignKey but I was just curious about for which cases the elseif was usefull implemented like this. Of course I would be happy to make a pull request if something is wrong here.

Cheers for the great package!

PS: I also think that the Laravel function getForeignKey should depend on the primary_key:

public function getForeignKey()
{
    return Str::snake(class_basename($this)).'_' . $this->primaryKey;
}

[dimsav]

Hi @ElMatella and thank you for sharing your thought. This is an interesting question. It seems that adding the table name to the relationship key makes sense. However the wished behavior can be different from project to project. If I remember correctly, this was implemented like this because it was convenient to the first developer that asked for this feature. Then others wanted a different value, so that's why $translationForeignKey was created.

A PR would make sense but that would be a breaking change and I wouldn't make a major release just for that. Also changing this in a major release would introduce nasty bugs to those who would not pay attention to the incompatibility.

[ElMatella]

I understand the idea. Maybe we could still keep it somewhere for the next major major release. As a matter of fact, someone made a pull request to the laravel framework core to change the framework behaviour on that subject, you can view the pull request here: laravel/framework#16396

As this is a Laravel Package, I assume that it would be a good idea to follow the framework and that it would not break any build if we integrate it when Laravel 5.4 is released.

This would simplificate to:

public function getRelationKey()
{
    if ($this->translationForeignKey) {
        return $this->translationForeignKey;
    } 

    return $this->getForeignKey();
}

[dimsav]

Indeed, we could add this for the Laravel 5.4 release.

[Gummibeer]

https://github.com/dimsav/laravel-translatable/releases/tag/v10.0.0