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

Use the name of the table to create the relation #542

Merged
merged 12 commits into from
Nov 20, 2023

Conversation

thiagotalma
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

samdark commented Oct 19, 2023

What if both relations are needed?

@thiagotalma
Copy link
Contributor Author

thiagotalma commented Oct 19, 2023

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
Copy link
Member

samdark commented Oct 20, 2023

Tests should be fixed.

@thiagotalma
Copy link
Contributor Author

thiagotalma commented Nov 6, 2023

Tests should be fixed.

@samdark Done!

With updated tests, the proposal can become clearer.

@thiagotalma
Copy link
Contributor Author

thiagotalma commented Nov 10, 2023

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

@xepozz
Copy link
Member

xepozz commented Nov 11, 2023

@bizley @terabytesoftw Could you please review?

@rhertogh
Copy link
Contributor

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
Copy link
Member

bizley commented Nov 13, 2023

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

@schmunk42
Copy link
Contributor

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
Copy link
Member

bizley commented Nov 13, 2023

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

@thiagotalma
Copy link
Contributor Author

Configuration in the module or on the gii screen?

@schmunk42
Copy link
Contributor

gii screen I think

@schmunk42
Copy link
Contributor

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

@thiagotalma
Copy link
Contributor Author

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

@schmunk42
Copy link
Contributor

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
Copy link
Contributor

rhertogh commented Nov 15, 2023

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
Copy link
Contributor Author

Done!

Thanks, @schmunk42

@schmunk42
Copy link
Contributor

@thiagotalma You're welcome.

LGTM now :)

@samdark samdark merged commit e65f3b2 into yiisoft:master Nov 20, 2023
11 checks passed
@samdark
Copy link
Member

samdark commented Nov 20, 2023

Thank you!

@thiagotalma thiagotalma deleted the patch-1 branch November 20, 2023 13:24
@thiagotalma thiagotalma restored the patch-1 branch November 20, 2023 13:32
@thiagotalma
Copy link
Contributor Author

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

@bizley
Copy link
Member

bizley commented Nov 21, 2023

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

@thiagotalma
Copy link
Contributor Author

@bizley
Done!
#544

@thiagotalma
Copy link
Contributor Author

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

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

Successfully merging this pull request may close these issues.

7 participants