Change null fields to be strictly compared when logging dirty fields

[adamlehmann]

When using $logOnlyDirty = true and an attribute is changing from null to false or 0 the change isn't currently logged as the spaceship operator just does a loose comparison.

I've added in code to check if either the new or old value is null and if so it will do a strict comparison check to allow that change to be logged.

Potentially this could be a optionally turned on per model by setting a property on the model.

[Gummibeer]

Thanks for your PR - it looks good. The only thing I think about if null is the only value that will fail because of this weak-comparison?

(null <=> false); // 0
('' <=> false); // 0
(0 <=> false); // 0
('0' <=> false); // 0
(1 <=> true); // 0
('1' <=> true); // 0
('hello world' <=> true); // 0
('1' <=> 1); // 0
('1.0' <=> 1); // 0
('1,1' <=> 1); // 0

http://sandbox.onlinephpfunctions.com/code/8f4a4741cee4ca556c6ce8f2bf81f03667c0936d

I bet that there are some more. So there are some examples that aren't good real-world ones but primary the last one shows how bad PHP is in weak-comparison. 😕
The ones before could be argumented that they are the same if the attribute has a type-cast. But the last one is some kind of real-world example but definitely another value.

My major problem with this is that this will be, in every case, some kind of BC. So would like to pass the decision part to @freekmurze how to handle this problem/bug.

[adamlehmann]

Yeah, originally I was just going to do strict checking for everything, however the tests were failing when I did that because $this->fresh() in method attributeValuesToBeLogged would return user_id as a string instead of an integer. So it would then log that user_id had been updated with a new value '3' and an old value 3.

However when implementing strict checking in my own project, it solves the problems I was having and doesn't have any issues with integers being returned as strings. I haven't really dug into why $this->fresh() is returning integers as strings in the tests but doesn't in my project. In my project they are just unsigned integers without any casting.

[adamlehmann]

A bit of digging and I believe it has to do with SQLite being used for the test database of this package. SQLite and MySQL where the MySQL Native Driver isn't enabled will return integers as strings by default. For my project I am using MySQL with the MySQL Native Driver enabled for both tests and the application.

If you dd($article) after first creating an article in the tests it will show all integers as integers but if you dd($article->fresh()) it will show the integers as strings (except for auto-incrementing integers).

[Gummibeer]

@adamlehmann thanks for your work here! :)

The difference between DB engines and the corresponding driver is enormous - our problem is that we have to support all of them.
Personal I define all types different from string in $casts. With this I have everytime a typesecure setup independent of DB.
I will also take a look in fresh() and if it could get replaced by anything else.

An idea could be to use the retrieved model event which is thrown after querying. This way we could store all attributes with the original value including the overloaded ones. My problem with this is that it will probably produce several useless queries if overloaded attributes do some.

So I will try to find a way to cast the old/fresh attributes the same way the current ones are casted to do typesafe comparison.

[adamlehmann]

Doing something like this works and all tests pass, although I'm not sure if it's the most elegant solution: https://gist.github.com/adamlehmann/8e8051386c4783748037022ee1090689/revisions

[AlexVanderbist]

Merging this one for now, feel free to open a new PR with the replacement/changes for the fresh() method if still relevant. Thanks!

[AlexVanderbist]

Also, I'm considering a bug fix for the use case as seen in the tests. Will be tagged together with the feature version for Laravel 6 tho