Problem to inherit yii\db\Query

rskrzypczak commented 5 years ago

The problem is that when you want to inherit after yii\db\Query class I have to also overwrite some functions that refer to new self(). Changing self to static should eliminate possible errors that might appear in such cases and it will maintain object consistency.

What steps will reproduce the problem?

https://github.com/yiisoft/yii2/blob/869c8aa22059f3aaa1dbdce0b74b849f0d7cb189/framework/db/Query.php#L466-L469

What is the expected result?

Additional info

Q	A
Yii version	2.0.15.3
PHP version	7.3.x
Operating system

samdark commented 5 years ago

OK. Feel free to do a pull request.

samdark commented 5 years ago

@rob006: It could be fixed by introducing another protected method for creating instance used by these two methods, so descendants could override it. Changing this to static directly in Query would break BC.

fcaldarelli commented 5 years ago

There are only 2 self references in yii\db\Query class:

@rskrzypczak I'm curious. What is the purpose to inherit Query class?

I'm agree with @samdark to introduce a new protected method to be overridden. Could classReference() be a good name for this new method?

samdark commented 5 years ago

There's alike thing in AR already: https://github.com/yiisoft/yii2/blob/master/framework/db/BaseActiveRecord.php#L1213

fcaldarelli commented 5 years ago

@samdark The instantiate method return an object, but in this case (and in general case) it would be better to get the class so we can pass a variable number of arguments to the constructor.

Here https://github.com/yiisoft/yii2/blob/869c8aa22059f3aaa1dbdce0b74b849f0d7cb189/framework/db/Query.php#L1285 it would be better pass a reference to the class.

protected static function classReference()
{
    return static::class;
}

public static function create($from)
{
    $currentClass = static::classReference();
    return new $currentClass([
        'where' => $from->where,
        'limit' => $from->limit,
        'offset' => $from->offset,
        'orderBy' => $from->orderBy,
        'indexBy' => $from->indexBy,
        'select' => $from->select,
        'selectOption' => $from->selectOption,
        'distinct' => $from->distinct,
        'from' => $from->from,
        'groupBy' => $from->groupBy,
        'join' => $from->join,
        'having' => $from->having,
        'union' => $from->union,
        'params' => $from->params,
    ]);
}

otherwise we can use instantiate output, getting the parameters inside it and calling the constructor, such as:

protected static function instantiate()
{
    $args = func_get_args();

    $class = new ReflectionClass(static::class);
    $instance = $class->newInstanceArgs($args);
    return $instance;

}

rob006 commented 5 years ago

@FabrizioCaldarelli That wouldn't help at all, since descendants could have different signature of constructor. That is the whole point of separate method. If that would not be a problem, we could just use static.

fcaldarelli commented 5 years ago

@rob006 but sometimes the constructor is called without parameters and sometimes with parameters. See the two links above.

rob006 commented 5 years ago

The second case could be replaced by setting properties in foreach or by Yii::configure(), passing params through constructor is not crucial. Alternatively $config could be added as argument:

protected static function instantiate(array $config = []) {
    return self($config);
}

fcaldarelli commented 5 years ago

@rob006 I thought that, but it would not be general.

Think if constructor required more then one parameter (not array).

rob006 commented 5 years ago

What about:

protected static function instantiate(self $instance, array $config = []) {
    return self($config);
}

$instance could be used to obtain other parameters for constructor (for example $modelClass in ActiveQuery).

fcaldarelli commented 5 years ago

$instance could be used to obtain other parameters for constructor (for example $modelClass in ActiveQuery).

The question is how have an instantiate method that can return an object with variable number of parameter, to solve both these points:

While we are at, I'll think at one more general solution, to apply at cases when we need to pass more than one generic parameters ($config array).

rob006 commented 5 years ago

The question is how have an instantiate method that can return an object with variable number of parameter, to solve both these points:

$command = static::instantiate($this)

return static::instantiate($from, [/* ... */]);

While we are at, I'll think at one more general solution, to apply at cases when we need to pass more than one generic parameters ($config array).

This array is the only thing available. I'll repeat: you cannot create dynamic signatures here, because dynamic signature of constructor is the main source of problems here. instantiate() should not expect anything except $config, because this would break LSP. Adding $instance here looks more like a hack than good API, but it should be flexible enough to cover all sane cases. I don't think that generalization is the goal here, this issue is more like creating BC workaround for design issue. This is very specific case, I don't think that this method would be useful for anything else.

yiisoft / yii2