Use the name of the table to create the relation

[thiagotalma]

Q	A
Is bugfix?	❌
New feature?	❌
Breaks BC?	❌

Use the real name of the table to create the relationship when there is a composite primary key.

Prevents the generation of random relationship names.

Turn this:

<?php

(...)

/**
 *
 * @property int $id
 * @property int $employee_id
 *
 * @property Employee $employee
 * @property Route $employee0 <---
 */
class Company extends \yii\db\ActiveRecord
{
    (...)

    /**
     * Gets query for [[Employee]].
     *
     * @return \yii\db\ActiveQuery
     */
    public function getEmployee()
    {
        return $this->hasOne(Employee::class, ['id' => 'employee_id']);
    }

    /**
     * Gets query for [[Employee0]]. <---
     *
     * @return \yii\db\ActiveQuery
     */
    public function getEmployee0() /* <--- */
    {
        return $this->hasOne(Route::class, ['employee_id' => 'employee_id', 'id' => 'route_id']);
    }
}

Into this:

<?php

(...)

/**
 *
 * @property int $id
 * @property int $employee_id
 * @property string $route_id
 *
 * @property Employee $employee
 * @property Route $route <---
 */
class Company extends \yii\db\ActiveRecord
{
    (...)

    /**
     * Gets query for [[Employee]].
     *
     * @return \yii\db\ActiveQuery
     */
    public function getEmployee()
    {
        return $this->hasOne(Employee::class, ['id' => 'employee_id']);
    }

    /**
     * Gets query for [[Route]]. <---
     *
     * @return \yii\db\ActiveQuery
     */
    public function getRoute() /* <--- */
    {
        return $this->hasOne(Route::class, ['employee_id' => 'employee_id', 'id' => 'route_id']);
    }
}

[thiagotalma]

The tests failed, but this is exactly the kind of thing PR should avoid:

['product_language', 'ProductLanguage.php', false, [
    [
        'name' => 'function getSupplier()',
        'relation' => "\$this->hasOne(Product::className(), ['supplier_id' => 'supplier_id', 'id' => 'id']);",
        'expected' => true,
    ],
    [
        'name' => 'function getSupplier0()',
        'relation' => "\$this->hasOne(Supplier::className(), ['id' => 'supplier_id']);",
        'expected' => true,
    ],
]],

This:

getSupplier() hasOne Product
getSupplier0() hasOne Supplier

Should be:

getProduct() hasOne Product
getSupplier() hasOne Supplier

[samdark]

What if both relations are needed?

[thiagotalma]

This:

getSupplier() hasOne Product
getSupplier0() hasOne Supplier

Should be:

getProduct() hasOne Product
getSupplier() hasOne Supplier

Look, I'm not eliminating the relationship.

Just renaming. Using the table name instead of the primary key.

[samdark]

Tests should be fixed.

[thiagotalma]

Tests should be fixed.

@samdark Done!

With updated tests, the proposal can become clearer.

[thiagotalma]

@samdark Is there any chance this change will be present in the next release?
Is there already a release date?

[xepozz]

@bizley @terabytesoftw Could you please review?

[rhertogh]

I think there should be a configuration option for this change for backwards compatibility. E.g. when used with https://github.com/schmunk42/yii2-giiant.

[bizley]

I would not want to keep the old way. @schmunk42 could you check if this change is problematic with your package?

[schmunk42]

One thing is, that the workflow with giiant is to regenerate base-models everytime your db changes, which would be problematic here, since the relations would change.

But another more important thing is, that if you have a table company with 2 FKs to Employee eg. president_id and vice_president_id, and you take the relation name from the table name you'll end up with getEmployee and getEmployee0. In this use-case you are better off with relation names from the column.

I'd not change this. At least not for yii2.
Or make it configurable.

[bizley]

Ok, I see. If we correct this in one case, the other one is broken. @thiagotalma could you make it configurable?

[thiagotalma]

Configuration in the module or on the gii screen?

[schmunk42]

gii screen I think

[schmunk42]

@thiagotalma There should be tests for both cases (original ones and the new one).

[thiagotalma]

Can anyone help me with the tests, please?
(Yes, it's a shame. I know)

[schmunk42]

You might need an additional param in your data provider:

public function testRelations($tableName, $fileName, $useClassConstant, $relations, $fromDestTable = false)
    {
        $generator = new ModelGenerator();
...
        $generator->generateRelationNameFromDestinationTable = $fromDestTable;

And use it in your new use-cases in relationsProvider():

            ['product_language', 'ProductLanguage.php', false, [
                [
                    'name' => 'function getSupplier()',
                    'name' => 'function getProduct()',
                    'relation' => "\$this->hasOne(Product::className(), ['supplier_id' => 'supplier_id', 'id' => 'id']);",
                    'expected' => true,
                ],
                [
                    'name' => 'function getSupplier0()',
                    'name' => 'function getSupplier()',
                    'relation' => "\$this->hasOne(Supplier::className(), ['id' => 'supplier_id']);",
                    'expected' => true,
                ],
            ],
         true // $fromDestTable
         ],

The existing ones should be kept/reverted.

[rhertogh]

But another more important thing is, that if you have a table company with 2 FKs to Employee eg. president_id and vice_president_id, and you take the relation name from the table name you'll end up with getEmployee and getEmployee0. In this use-case you are better off with relation names from the column.

I'd not change this. At least not for yii2. Or make it configurable.

I was thinking about this some more since I encountered a similar situation today.
In my opinion the best option would be to use the name of the foreign key itself.
In the example from @schmunk42 above the foreign keys probably would be named "company_president" and "company_vice_president" respectively (and perhaps prefixed with "fk_" based on the used convention).
If we allow the configuration of a closure, the application can generate the base name ($key parameter for Generator::generateRelationName()). That callback function should look something like:

function ($foreignKey) {
   // some custom base name determination
   return $myCustomBaseName;
}

So my proposal would be to make it indeed configurable as discussed above and add a third option.
The Generator::$generateRelationNameFrom, would accept these options:

• (string) COLUMN_NAME
• (string) TARGET_TABLE_NAME
• (closure) function(){ }

[thiagotalma]

Done!

Thanks, @schmunk42

[schmunk42]

@thiagotalma You're welcome.

LGTM now :)

[samdark]

Thank you!

[thiagotalma]

Guys, it looks like there was a problem with the merge.
Changes from PR #537 have been lost.

[bizley]

Oh, indeed. @thiagotalma could you prepare another PR correcting this?

[thiagotalma]

@bizley
Done!
#544

[thiagotalma]

Hello guys!

What do you think about releasing the new version?
The milestone is already cleared.

I'm just waiting for these changes to move forward with my project.

@samdark