owen-it / laravel-auditing

Record the change log from models in Laravel
https://laravel-auditing.com
MIT License
3.01k stars 387 forks source link

Reverting IP resolver caused issues (breaking) #806

Closed MortenDHansen closed 1 year ago

MortenDHansen commented 1 year ago

@erikn69 Reverting this change caused the package to break the recording of all activities. The Reqeust::ip() returns the IP address formatted as 127.0.0.1:44444 format (adding the port number at the end) while the migration that generates the audit table expects a flat IP format. This throws a fatal error when performing the insert operation because of the SQL validation.

A workaround is having a custom IpResolver, but it would be good if the package handles the issue as other systems already deployed might be impacted by this change on the next update/re-deploy.

Originally posted by @APavlov2 in https://github.com/owen-it/laravel-auditing/issues/743#issuecomment-1477636727

By oversight it seems we created a breaking change. We should probably add a guard in IP resolver that checks for a valid value before attempting to write.

(and apparently we are missing one or more tests in this regard 😞 )

parallels999 commented 1 year ago

By oversight it seems we created a breaking change.

It only returned to the original functionality

the package has been using Request::ip() since five years ago.

Originally posted by @ceejayoz https://github.com/owen-it/laravel-auditing/pull/743#issuecomment-1477655641

So, for avoid port, what about just explode

return explode(':', (string) Request::ip())[0];

https://github.com/owen-it/laravel-auditing/blob/f7da93ef11a28a6dc656168d0479a37bd3843b59/src/Resolvers/IpAddressResolver.php#L9-L15

MortenDHansen commented 1 year ago

Oh, jeez. My memory is terrible ...

I just looked at https://github.com/owen-it/laravel-auditing/pull/743 again. Yeah, that was not supposed to be merged 😄

Right! - i'll stop trying to remember more than 4 days back!