-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Changed behavior of the yii\base\BaseModel::attributes()
method in 2.0.46
#19546
Comments
@WinterSilence would you please take a look? |
The only place where We could reuse |
I wonder if |
@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
do you test it? public your benchmarks |
It is your "optimization", you should provide benchmark in #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. |
I would like to understand on the basis of what do you draw such conclusions if you don't have any benchmarks?
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...
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....
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
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. |
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 |
Should be reverted IMHO. |
@bizley would you please handle reverting it? |
Sure, this was only #19309 right? |
Yes, I think so. |
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?
What is the expected result?
What do you get instead?
Additional info
This change came after PR #19309
The text was updated successfully, but these errors were encountered: