yii2
yii2 copied to clipboard
yiisoft
Changed behavior of the `yii\base\BaseModel::attributes()` method in 2.0.46
It looks like there was a breaking change in yii\base\Model::attributes() method. In 2.0.45 it returned uninitialized properties, but in 2.0.46 such properties are not returned.
What steps will reproduce the problem?
use yii\base\Model;
class TestModel extends Model
{
public string $id;
}
$model = new TestModel();
var_dump($model->attributes());
What is the expected result?
array(1) {
[0]=>
string(2) "id"
}
What do you get instead?
array(0) {
}
Additional info
This change came after PR https://github.com/yiisoft/yii2/pull/19309
| Q | A |
|---|---|
| Yii version | 2.0.46 |
| PHP version | 8.1 |
| Operating system | Linux |
The only place where attributes() itself is tested:
https://github.com/yiisoft/yii2/blob/master/tests/framework/base/ModelTest.php#L524
We could reuse Speaker https://github.com/yiisoft/yii2/blob/161526cd41beee62d0910333c3bd96034cf73186/tests/data/base/Speaker.php for testing this, by checking for firstName, lastName on a newly created instance.
I wonder if gii works with 2.0.46, since the bug described here would render it completely unusable - untested, it's just a thought.
@samdark this code generates error because property not defined by default or in constructor.
It should be reverted in that case.
I would also be more cautious with such "optimization" PRs in the future, since they often produces BC breaks without any noticeable performance boost.
@rob006 i mean @flight643 example
without any noticeable performance boost
do you test it? public your benchmarks
do you test it? public your benchmarks
It is your "optimization", you should provide benchmark in https://github.com/yiisoft/yii2/issues/19546. Although it that case it does make much since new behavior looks incorrect, so it should be reverted even if it is significantly faster.
@rob006
It is your "optimization", you should provide benchmark
I would like to understand on the basis of what do you draw such conclusions if you don't have any benchmarks?
it does make much since new behavior looks incorrect
what behavioral changes are you talking about? can you post correct example?
My Two cents,
Firstly kudos for wintersilence for working on optimisations, this is always a great thing and shows the base is moving forward. Its just in this case some unintended consequences have been hit. If we look at the attributes() function description, it states...
* Returns the list of attribute names.
*
* By default, this method returns all public non-static properties of the class.
* You may override this method to change the default behavior.
Notice the word "all".
Now with the new code we are moving from using Reflections which I agree with wintersilence is historically something one wants to move away from (although one could argue nowadays its prob been optimized so much its not so bad). To using Yii getObjectVars() which uses PHP core get_object_vars() and this function states....
* @return array an associative array of defined object accessible non-static properties
* for the specified <i>object</i> in scope. If a property have
* not been assigned a value, it will be returned with a null value.
I suspect the line we are interested in is "If a property have not been assigned a value... it will be returned with a null value"
This is me just doing a quick 5 min look, so could be very wrong :-) But if right then need to either
- change the comment in the attributes() function by removing "all" and stating "properties that have been assigned a value"
- update Yii getObjectVars() with say a flag indicating give me all object vars including ones that dont have a value.
- or just revert but add a comment in the attributes function saying this can be optimised at a later stage?
As this is probably being used in thousands of installations I think best to revert. Again my two cents :-)
@markch123 typed properties must be initialized before access i.e. (new TestModel())->id generates error
Was there some consensus on if this is a bug or not or something that needs extra documentation?
We’re seeing Craft plugins that have broken as a result of this change, as they had typed properties w/out default values, which were relying on attributes() to set themselves. I would consider it a breaking change that should be fixed ASAP.
