active-record
active-record copied to clipboard
yiisoft
ActiveRecord::__get priority
Currently the activerecord implementation gives a higher priority to attributes (and relations) than it gives to setters and getters.
Often it is desired to overwrite an attribute (from the PHP point of view) using a getter and setter; since we can directly set an attribute using setAttribute.
Currently there are some issues with how writing to attributes is handled:
- There are several places where the _attributes array is directly accessed, this means I cannot control it by overriding setAttribute (since that one gets bypassed in populate and __get / __set).
- I cannot override a database field using getters and setters in my model, for example getCreated() to return a datetime object instead of a string. (Currently it can be done by using other names for the database field and the getter, but that is not desirable).
Some improvements could be made:
- Only write to $_attributes via setAttribute.
- Prioritize getXX and setXX over relations in magic getters and setters.
Currently ActiveRecord breaks a contract set by Object; from the docs:
Object is the base class that implements the property feature.
- That's OK.
- Doesn't sound right. If you have the same name, it's conflicting anyway so it doesn't really matter what's preferred. It's not right anyway.
Why is 1 OK? Should I not be able to control what happens to my database fields?
How would I neatly implement type casting for example? (Not based on column type, but more generic, say I want integers in some field to be replaced with a value object, say, Meter).
To prevent / override reading / writing the database field directly I would have to override lots of function, reimplement ancestor behavior (since I want to skip BaseActiveRecord but I do want the Object getters / setters).
1 . There are several places where the _attributes array is directly accessed, this means I cannot control it by overriding setAttribute (since that one gets bypassed in populate and __get / __set).
I am unsure it can be done, and unsure it should. It simply has no sense: these provides access to the raw database entity attributes, which are necessary while inserting/updating the record. There is nothing to change here.
- I cannot override a database field using getters and setters in my model, for example getCreated() to return a datetime object instead of a string. (Currently it can be done by using other names for the database field and the getter, but that is not desirable).
This is not a correct way. If you override attribute access making some conversions on it. It will no way to save it actual value to database, especially if you insist on '1' implementation.
For example: imagine you have overridde getter for created_date in the way it returns DateTime object - so you are unable to get actual database value of it on 'insert' or 'update' operation, because $this->created_date always return object, while it should be integer or string for the database table.
You should create an alias for the attributes, for this purpose, it is alreay supported:
public function getCreatedDate()
{
return new \DateTime($this->created_date);
}
public function setCreatedDate(\DateTime $date)
{
$this->created_date = $date->toTimestamp();
}
You can setup validators for these virtual fields just like regular attributes. I can't see any problem.
Currently ActiveRecord breaks a contract set by Object; from the docs: Object is the base class that implements the property feature.
Yes. But there is no such thing as property access contract. It could be changed by child classes. For example: https://github.com/yiisoft/yii2/blob/master/framework/di/ServiceLocator.php#L68 https://github.com/yiisoft/yii2/blob/master/framework/base/DynamicModel.php#L81
You don't compain on these changes, do you?
It also has more sugnificant break of Object contract, see #583.
Regardless if you think I should be overriding them; currently the access is being duplicated across the ActiveRecord class. That should be enough reason to change it.
Giving other examples of stuff I don't complain about is not a valid argument in any discussion.
For the specific case of value objects, as long as they implement __toString properly they can be saved to the database just fine. (Note that currently Yii2 already does some typecasting which could in the future easily be extended to utilize some interface, so that for example if an object implements the DatabaseSerializable interface it gets serialized via some specific function from the interface..)
In short, it is a code clean up that provides me with more options as a side effect. If you do not want me to override a function mark it final, don't duplicate code and use the duplication as an excuse for not letting me override a function.
Regardless if you think I should be overriding them; currently the access is being duplicated across the ActiveRecord class. That should be enough reason to change it.
Perhaps, but it seems it was some reason behind the current implementation - I just can not remember, which exactly. Need to be careful here.
For the specific case of value objects, as long as they implement __toString properly they can be saved to the database just fine.
For the objects - perhaps, but what will prevent you from other manipulations? What if someone will change the date format from 'Y-m-d' to 'Y/d/m' - he will receive the unexpected result. And perhaps will notice it only after some time.
There always SHOULD BE a way to access RAW table attributes, while you are talking about its possible removal.
Giving other examples of stuff I don't complain about is not a valid argument in any discussion.
Just as Currently ActiveRecord breaks a contract set by Object; from the docs is not a argument in any discussion too.
Lol, always fun to have these debates with you @klimov-paul !
Perhaps, but it seems it was some reason behind the current implementation - I just can not remember, which exactly. Need to be careful here.
If there is a reason for implementation decisions, then document it in the source code to prevent these discussions in the future ^^
For the objects - perhaps, but what will prevent you from other manipulations? What if someone will change the date format from 'Y-m-d' to 'Y/d/m' - he will receive the unexpected result. And perhaps will notice it only after some time.
These are coders we are talking about, they can break it if they want to; I can break the current implementation in exactly the same way, since I can assign objects to AR attributes without any of my changes...
There always SHOULD BE a way to access RAW table attributes, while you are talking about its possible removal.
Why should there be a way? As far as I can tell there is no way for me to access them without being typecasted in Db\Column, so "raw" is relative in any case. If I want a way to access raw attributes I can just call the parent implementation of getAttribute right?
Just as Currently ActiveRecord breaks a contract set by Object; from the docs is not a argument in any discussion too.
It is a perfectly valid argument: it means either the documentation is (wrong, incomplete or unclear) or the code is wrong.
Perhaps, but it seems it was some reason behind the current implementation - I just can not remember, which exactly. Need to be careful here.
Perhabs consistency between $_oldAttributes & $_attributes
Have updated the PR, it now passes all tests and I have even added a new test for getDirtyAttributes to test if the attributes are being tracked properly.
I can tomorrow. Summary: ar attributes are only read/written to via functions that can be overridden.
On Fri, Nov 20, 2015, 21:39 Tom Worster [email protected] wrote:
Can someone summarize what just changed?
Because it broke yii2-dynamic-ar.
[image: Alt text] https://camo.githubusercontent.com/3d1bc731e37780a41387083eb2585eae275bb3cb/68747470733a2f2f6d6f6e6f736e61702e636f6d2f66696c652f63704c6e71586c526e487a4d576939735468625a516c48486c755473574a2e706e67
— Reply to this email directly or view it on GitHub https://github.com/yiisoft/yii2/issues/9656#issuecomment-158517986.
I don't think non-BC change should be in 2.0.7.
I don't understand the discussion above, but changing BaseActiveRecord::__get() so that it calls the instance's getAtttibute() is a breaking change, at the very least.
I am not at my PC. But this change does not change the public api and passes all tests. I'll try reproducing the error tomorrow, and will add tests.
On Fri, Nov 20, 2015, 22:00 Tom Worster [email protected] wrote:
I don't think non-BC change should be in 2.0.7.
I don't understand the discussion above, but changing BaseActiveRecord::__get() so that it calls the instance's getAtttibute() is a breaking change, at the very least.
— Reply to this email directly or view it on GitHub https://github.com/yiisoft/yii2/issues/9656#issuecomment-158522089.
I have not reviewed this PR before because I had no time for it. I do not think that this change should be made in 2.0.x and also not sure about 2.x.0. It changes a lot about how AR internals work and breaks existing code that deals with attribute handling.
It shouldn't though.. I can't make it tonight, will test Tom's scenario Monday at the latest.
On Sat, Nov 21, 2015, 16:26 Carsten Brandt [email protected] wrote:
I have not reviewed this PR before because I had no time for it. I do not think that this change should be made in 2.0.x and also not sure about 2.x.0. It changes a lot about how AR internals work and breaks existing code that deals with attribute handling.
— Reply to this email directly or view it on GitHub https://github.com/yiisoft/yii2/issues/9656#issuecomment-158652177.
@SamMousa when you get to reviewing this, please read the DynamicActiveRecord Design Principle—it's very short.
Implementation: DynamicActiveRecord::getAttribute() immediately calls parent::__get() expecting it to throw InvalidParamException if the named attribute does not exist. It catches the exception and implements the attribute itself. Recursion if BaseActiveRecord::__get() immediately calls the instance's getAttribute().
Will do.
On Sat, Nov 21, 2015, 17:57 Tom Worster [email protected] wrote:
@SamMousa https://github.com/SamMousa when you get to reviewing this, please read the DynamicActiveRecord Design Principle https://github.com/tom--/yii2-dynamic-ar#design-principle—it's very short.
Implementation: DynamicActiveRecord::getAttribute() immediately calls parent::__get() expecting it to throw InvalidParamException http://www.yiiframework.com/doc-2.0/yii-db-baseactiverecord.html#__get%28%29-detail if the named attribute does not exist. It catches the exception and implements the attribute itself. Recursion if BaseActiveRecord::__get() immediately calls the instance's getAttribute().
— Reply to this email directly or view it on GitHub https://github.com/yiisoft/yii2/issues/9656#issuecomment-158661967.
Hi @tom-- ,
I've looked into the issue and into DynamicActiveRecord. First of all, nice project I've made something like that myself using a kind of JsonBehavior that exposes attributes dynamically!
Okay, so before my changes the routing of __get went roughly like this:
- Check if
$this->_attributeshas the key, if so return it (BaseActiveRecord). - Else check if
$this->_relatedhas the key, if so return it (BaseActiveRecord). - Else check getter exists, if so return it. (Component)
After that, if result type is ActiveQuery run the query and cache the results in $this->_related and return them instead of the query object.
In this flow, the getAttribute function is not used, meaning that if you wanted to adapt AR attribute handling (like in your project) you would have to override both __set, __get and getAttribute / setAttribute.
Since $_attributes is private you cannot write / read from it directly though.
After my patch, instead, __get and __set always try the attribute getters first, this means in your code the only "hooks" you need are getAttribute, hasAttribute and setAttribute.
Currently you have infinite recursion because your __get calls getAttribute, which calls "my" __get, which calls your getAttribute.
So TLDR; the patch does require some changes on your end but it will actually make the code flow of both BaseActiveRecord and DynamicActiveRecord simpler!
So TLDR; the patch does require some changes on your end but it will actually make the code flow of both BaseActiveRecord and DynamicActiveRecord simpler!
this implies that it is a BC break and should not be included in 2.0.x. We may apply it to 2.1 though.
Well, any change can be BC breaking if a descendant class overrides a function and then calls parent:: on another function... That is just a recipe for disaster when any change is made in the parent class.
we can not guarantee that we avoid breaking BC in all the cases but this one is obvious. 2.0.x releases of Yii are meant to be non breaking changes so we can not release 2.0.7 with this change as it is now. If we come up with a non breaking alternative it may go into 2.0.7 otherwise it will have to wait for 2.1.0
That's okay, I don't mind that and obviously that is your perogative. I'm just saying that if it really is a BC break there should be tests for it, there are and they pass. Since they test the public API.
The failing implementation here is DynamicActiveRecord, which is using knowledge that is not part of the public API. Calling a parent implementation of another function is not good practice, it assumes knowledge about the inner workings of the parent class.
I had no time to look at it yet, but it seems to me that in the current state it does not fit for a 2.0.x release.
API docs for Object, Component and BaseActiveRecord say that __get() throws an exception if the thing doesn't exist. __get() is a public method. DynamicActiveRecord::getAttribute() relies on this. I don't understand.
Afaict, unit tests for the exceptions exist for Object, Component but not ActiveRecord.
@tom-- , this implementation still throws that exception, however it calls $this->getAttribute to do the check.
Since your getAttribute again calls parent::__get() you get an infinite loop. Note that this can be easily fixed, but that is not the point of this dicussion.
I think the discussion is / should be more generic; if this is defined as BC breaking then any internal change, ie calling $this->b() from $this->a(), when it did not use to before, breaks BC.
Since someone might have an implementation of $this->b() that calls $this->a()...
So, iiuc, the "knowledge" that DynamicActiveRecord::getAttribute() relies on is: BaseActiveRecord::__get() does not call $this::getAttribute() before xyz.