-
Notifications
You must be signed in to change notification settings - Fork 46
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
Fix incompatible type matching #187
Conversation
[ci skip] [skip ci]
I have added some test-script for typecasting of non-string type's "" into null. The point here is that any non-string type with value of "" is invalid and is probably a null. |
Those cases seem to be cleaning up after improper use of casting, so technically we could say those are unnecessary, but create a lot of problems around Agile Data. If those fixes create a problem, please provide examples. |
$m['name'] = '4.28'; | ||
$this->assertEquals([], $m->dirty); | ||
$m['name'] = '5.28'; | ||
$this->assertNotEquals([], $m->dirty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why name didn't become dirty here ???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it did, hence the assertNotEquals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah sorry :D
Currently, if the field value is "4" and you change it to 4 (int) then this makes field dirty. To avoid this, hotfix was added: 33377f7
Unfortunately this resulted in some bad side-effects (not found by tests) reported here: #184 (comment)
This PR is properly addressing the issue, by using strict comparison unless we are dealing with stringified numbers. Also it adds several tests, so if there are more test-cases that are affected, then please add.
The original problem was caused because hasOne('blah', ); will use non-integer fields even for persistence SQL where
id
fields use int() by default. Still, we can't be fully sure about that and without typecasting SQL response comes back as string.