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

AR relation scope #1579

Closed
Ragazzo opened this issue Dec 19, 2013 · 19 comments
Closed

AR relation scope #1579

Ragazzo opened this issue Dec 19, 2013 · 19 comments
Labels
status:to be verified Needs to be reproduced and validated. type:bug Bug
Milestone

Comments

@Ragazzo
Copy link
Contributor

Ragazzo commented Dec 19, 2013

In Yii1 i can write following code and it will work:

        $model = Users::find()->with([
            'SomeOtherUsers' => [
                'scopes' => [
                    'byId' => [2],
                ],
            ],
        ])->all();

Main idea is that i can chain scopes and apply conditions in the way how i want it to be. In Yii2 its not working, i got

Warning – yii\base\ErrorException 
call_user_func() expects parameter 1 to be a valid callback, array must have exactly two members

in ActiveQueryTrait.php:196 line. I think we should keep feature of Yii1 when in with() for some relations i can set what scopes/limit/order should be bound with this relation. Or show how to do it correctly without losing this features? Example in the guide with

$customers = Customer::find()->limit(100)->with([
    'orders' => function($query) {
        $query->andWhere('subtotal>100');
    },
])->all();

is awful. I also suggest to exclude $query from params list and make it self::getQuery() if it is possible.

@qiangxue
Copy link
Member

See updated doc: 65d72eb

I don't think we should drop the $query parameter in favor of self::getQuery(). It is almost certain that you will need to manipulate the query object in the anonymous function. Why should we drop it?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 19, 2013

I don't think we should drop the $query parameter in favor of self::getQuery(). It is almost certain that you will need to manipulate the query object in the anonymous function. Why should we drop it?

i dont like to write manually each time, its like writing self in python, but there it is because of how objects created and here it is just kind of design lack. Thanks for updating docs, but how to set for example limit, can you add to the docs example where we are calling scope (already added) and adding some other params (limit, orderBy, etc) in same time for the same relation?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 19, 2013

Also is that a bug

        $model = Users::find()->with([
            'SomeOtherUsers' => function($q){
                $q->byId(2); #->limit(10)->orderBy('user_id')
            },
        ])->one(); //all()

when i query like this i got in debugger info:

1.0 ms  SELECT * FROM "some_other_user" WHERE "user_id"=1
D:\docs\Server\www\basic\controllers\SiteController.php(60)
1.0 ms  SELECT * FROM "some_other_user" WHERE (id = 2) AND ("user_id"=1)
D:\docs\Server\www\basic\controllers\SiteController.php(57)
1.0 ms  SELECT * FROM "tbl_user"
D:\docs\Server\www\basic\controllers\SiteController.php(57)

and in $model->SomeOtherUsers i got all rows from related table. Why does it query all rows and for what this first extra query?
Relation is generated by gii:

    /**
     * @return \yii\db\ActiveRelation
     */
    public function getSomeOtherUsers()
    {
        return $this->hasMany(SomeOtherUsers::className(), ['user_id' => 'id']);
    }

And in the opposite AR:

    /**
     * @return \yii\db\ActiveRelation
     */
    public function getUser()
    {
        return $this->hasOne(User::className(), ['id' => 'user_id']);
    }

    public static function byId($query,$id)
    {
        $query->andWhere('id = :id',[':id' => intval($id)]);
    }

Even when i change id in query, for example to 3 i got sql:

SELECT * FROM "some_other_user" WHERE (id = 3) AND ("user_id"=1)

but still all rows a quired from related table and in $model->SomeOtherUsers i got all models like there were no conditions.
tables structure: users like in Yii2 advanced template, other table just holds user_id and some_attribute_key attribute.

@samdark samdark reopened this Dec 19, 2013
@samdark
Copy link
Member

samdark commented Dec 19, 2013

Queries generated look a bit suspicious...

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 20, 2013

Was testing this on Windows 7 / php 5.4 / postgresql 9.3 on yii2-basic template.
Migrations are:

  1. like in yii2-advanced, for tbl_user.
  2. second migration:
use yii\db\Schema;

class m131219_152916_tbl_some_other_user extends \yii\db\Migration
{
    public function safeUp()
    {
        $tableOptions = null;
        if ($this->db->driverName === 'mysql') {
            $tableOptions = 'CHARACTER SET utf8 COLLATE utf8_general_ci ENGINE=InnoDB';
        }

        $this->createTable('some_other_user', [
            'id' => Schema::TYPE_PK,
            'user_id' => Schema::TYPE_INTEGER. ' NOT NULL',
            'attribute_key' => Schema::TYPE_STRING . '(32) NOT NULL',
        ], $tableOptions);

        $this->addForeignKey('some_other_user_users_fk','some_other_user','user_id','tbl_user','id');

    }

    public function safeDown()
    {
        $this->dropTable('some_other_user');
    }
}

@qiangxue
Copy link
Member

It's not quite clear to me what the problem is. What are the query statement and what are the corresponding SQLs (you listed three, but there should only be two)?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 20, 2013

the problem is that when i query with relation and apply some scopes/limits to relations they are dont apply (and query all rows from related table), looks like. And yes i dont know why it generates 3 queries (they are taken from debug) instead of 2. You can this on basic boilerplate, there is an info about migrations for tables and relations.
I expect $model->someOtherUsers[0] to be a AR with id 2, but it just returns all records without applying scopes and limits as i've described above.

@qiangxue
Copy link
Member

The call stack line number for the three SQL statements are different. Are you sure they are all for the same query statement?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 20, 2013

yes, my controller only have this (modified it a little since was posted here, but issue is still the same):

<?php

namespace app\controllers;

use Yii;
use yii\web\AccessControl;
use yii\web\Controller;
use yii\web\VerbFilter;
use app\models\LoginForm;
use app\models\ContactForm;
use app\models\Users;

class SiteController extends Controller
{
    public function behaviors()
    {
        return [
            'access' => [
                'class' => AccessControl::className(),
                'only' => ['logout'],
                'rules' => [
                    [
                        'actions' => ['logout'],
                        'allow' => true,
                        'roles' => ['@'],
                    ],
                ],
            ],
            'verbs' => [
                'class' => VerbFilter::className(),
                'actions' => [
                    'logout' => ['post'],
                ],
            ],
        ];
    }

    public function actions()
    {
        return [
            'error' => [
                'class' => 'yii\web\ErrorAction',
            ],
            'captcha' => [
                'class' => 'yii\captcha\CaptchaAction',
                'fixedVerifyCode' => YII_ENV_TEST ? 'testme' : null,
            ],
        ];
    }

    public function actionIndex()
    {
        $model = Users::find()->with([
            'SomeOtherUsers' => function($q){
                $q->byId(3); #->limit(10)->orderBy('user_id')
            },
        ])
        ->where('id=:id',[':id' => 1])
        ->one();
        #var_dump($model);
        #print_r($model->attributes);
        print_r($model->someOtherUsers);
        #exit();
        return $this->render('index');
    }

    public function actionLogin()
    {
        if (!\Yii::$app->user->isGuest) {
            $this->goHome();
        }

        $model = new LoginForm();
        if ($model->load($_POST) && $model->login()) {
            return $this->goBack();
        } else {
            return $this->render('login', [
                'model' => $model,
            ]);
        }
    }

    public function actionLogout()
    {
        Yii::$app->user->logout();
        return $this->goHome();
    }

    public function actionContact()
    {
        $model = new ContactForm;
        if ($model->load($_POST) && $model->contact(Yii::$app->params['adminEmail'])) {
            Yii::$app->session->setFlash('contactFormSubmitted');
            return $this->refresh();
        } else {
            return $this->render('contact', [
                'model' => $model,
            ]);
        }
    }

    public function actionAbout()
    {
        return $this->render('about');
    }
}

@qiangxue
Copy link
Member

Try changing 'SomeOtherUsers' => function... to 'someOtherUsers' => function...?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 20, 2013

worked like a charm :D yeah, i think this should be documented or maybe somehow fixed in code. Also when generating AR i got error in relation i got

return $this->hasMany(SomeOtherUser::className(), ['user_id' => 'id']);

not in plural mode, i think this is because of Yii2 only stands for single AR names. But info about scope name is not documented/fixed anywhere.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 20, 2013

also looks like if i try to get parent relation it quires new sql, i think this should be fixed:

var_dump($model->someOtherUsers[0]->user->attributes == $model->attributes);

$model->someOtherUsers[0]->user will trigger another sql request, according logs:

2.0 ms  SELECT * FROM "tbl_user" WHERE id=1 #this is for Users::find()
D:\docs\Server\www\basic\controllers\SiteController.php(59)
1.0 ms  SELECT * FROM "some_other_user" WHERE (id = 2) AND ("user_id"=1) #this is for related someOtherUsers
D:\docs\Server\www\basic\controllers\SiteController.php(59)
1.0 ms  SELECT * FROM "tbl_user" WHERE "id"=1 #this one for $model->someOtherUsers[0]->user
D:\docs\Server\www\basic\controllers\SiteController.php(63)

@qiangxue
Copy link
Member

i think this should be documented or maybe somehow fixed in code.

Yes, this needs to be fixed in Yii by throwing an exception.

I got .... not in plural mode

This is expected due to limited knowledge of Gii in this case. You need to fix it manually.

$model->someOtherUsers[0]->user will trigger another sql request, according logs:

This is also expected. By no means Yii will attempt to populate parent relation when you don't request for it.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Dec 20, 2013

This is also expected. By no means Yii will attempt to populate parent relation when you don't request for it.

but maybe we can set it when parent is invoked with its child (or it is not needed because of cycle references)?

@qiangxue
Copy link
Member

The parent and child relations could be defined differently.

@cebe Do you still remember why we require relation names to be case sensitive? It used to be case insensitive.

@klimov-paul
Copy link
Member

@qiangxue, see #1125

@qiangxue
Copy link
Member

@klimov-paul Thanks. Looking through #1125, I still don't remember why we should make relation names case sensitive.

To fix this issue, if we keep relation names to be case sensitive, then we need to use method reflection to detect case sensitivity problem in places such as getRelation(), __get(), which is not very trivial.

@cebe
Copy link
Member

cebe commented Dec 20, 2013

I remember we did it becaue strtolower did not give a good result when we returned $this->_related back for relation names in camel case. Would be good to fix this somehow. Being case sensitive but not storing lowercase only in $this->_related. No idea how to solve it right now.

@qiangxue
Copy link
Member

I see. It's because of getPopulatedRelations(). I may introduce a new method hasRelation().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:to be verified Needs to be reproduced and validated. type:bug Bug
Projects
None yet
Development

No branches or pull requests

5 participants