Fix - BC Hook constants, Model ArrayAccess #31
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #31 +/- ##
==============================================
+ Coverage 62.27% 80.43% +18.15%
- Complexity 115 131 +16
==============================================
Files 5 5
Lines 281 322 +41
==============================================
+ Hits 175 259 +84
+ Misses 106 63 -43
Continue to review full report at Codecov.
|
src/Controller.php
Outdated
| @@ -278,12 +278,12 @@ public function getDiffs(Model $m) | |||
| } | |||
|
|
|||
| // don't log DSQL expressions because they can be recursive and we can't store them | |||
| if ($original instanceof Expression || $m[$key] instanceof Expression) { | |||
| if ($original instanceof Expression || $m->getField($key) instanceof Expression) { | |||
There was a problem hiding this comment.
i just replace with what i think is correct, i look further into it to be sure is not a useless code
There was a problem hiding this comment.
I believe it should be $m->get($key) instanceof Expression here
There was a problem hiding this comment.
it looks for the type of Field, we already have because is get few lines before using
$f = $m->hasField($key);so i take directly $f
There was a problem hiding this comment.
@georgehristov is right that original $m[$key] === $m->get($key)
$f = $m->hasField($key); will be deprecated no later than today :D in atk4/data#616 (more precisely it will be still valid, but it will return strictly bool only)
There was a problem hiding this comment.
getField() strictly returnds instance of Field. Check if Expression is some extended class of Field.
Value on the other side can be anything - because of load/save typecasting.
There was a problem hiding this comment.
that was introduced to avoid Expression to be included in audit.
this is the PR : b1e1af4#diff-3840aae95575d417f513143775141e9dR133
I clear don't remember if the value data was stored differently at that time, but now i think we have to get the model field type and compare to the Field_SQL_Expression
I'm on removing even hasFieldwhen is used to get the field and not to check for boolean presence
There was a problem hiding this comment.
Basically what this if should do is it should not audit any Expression field because expressions are not raw data - they are build based on raw data and are read-only. So no point of auditing them and also in some cases they can be recursive and then ... everything goes to hell if we try to audit them :)
There was a problem hiding this comment.
@georgehristov is right that original
$m[$key]===$m->get($key)
Expr. field will usually have field of type Field\Callback, but simply update the code test $m->get($key) instanceof Expre... like before, that makes sense.
There was a problem hiding this comment.
condition removed. Expression field has $f->read_only === true
8dc8a17 to
82c1a36
Compare
|
@mvorisek i think there are problems in atk4/data develop i recheck tests later |
22e5b3b to
a4703b4
Compare
|
|
I will finalize this now. If some additional tweaks will be needed then we can do this in new PR. |
There was a problem hiding this comment.
I'm happy as it is now. Thanks @abbadon1334 and @mvorisek for your good work here!
If you find some more issues with this then please start new branch because this one is already much to big to normally review :)
Merging.
Fixed errors due to recent BC in develop