Fix for issue #486 -

[specialtactics]

remove auditable_id integer cast, to allow for non-integer primary key models

[specialtactics]

Wondering if I can get any comments about this from you @squiaios, it's blocking our upgrade to Laravel 5.8 (and I'm sure we can't be the only ones).

[Namoshek]

Why does this break non-integer PKs? If you override the model in your code to use another type of PK, you can also override the $casts. In my opinion, this should not be changed as it would force others (like me) that are using the default configuration to add this cast manually in a customized model.

[specialtactics]

@Namoshek This is a vendor model, you can't just override it. The reason this breaks non-integer PKs is because it casts UUID PKs (or any string PKs) as integer, thereby changing them completely. I added a test in my PR which demonstrates this.

The cast shouldn't be necessary in any case - because auditable_id could be of any type in theory (but in particular integer or string), so this package shouldn't make assumptions as to it's type - especially given Laravel/Eloquent itself supports string key types directly.

So you understand, if this package casts auditable_id as integer, it will not work with what Laravel directly supports out of the box (ie. string primary keys).

[Namoshek]

Of course you can override it. It is the first option in the config:

laravel-auditing/config/audit.php

Lines 5 to 14 in eff1fe2

	/*
	|--------------------------------------------------------------------------
	| Audit Implementation
	|--------------------------------------------------------------------------
	|
	| Define which Audit model implementation should be used.
	|
	*/
	'implementation' => OwenIt\Auditing\Models\Audit::class,

In fact, I'm using a custom implementation of the model myself as I'm using a different base model for all of my models due to some added functionality. 👍

[specialtactics]

@Namoshek So can you explain why someone would need to override this model for acquire it's compatibility with default Laravel functionality?

Given it's been working with this functionality for the longest time, do you think it's appropriate to have just suddenly broken it?

If you have some special case where it doesn't work for you, then perhaps you should override it yourself, but the way it is in my PR works for all keys of all major databases (Postgres, MySQL, MariaDB, etc).

[Namoshek]

Yes, I do have a reasoning why it should be an integer cast:

• The default migration is for an integer column. If you adjust the migration, you can or should also adjust the model.
• The default Eloquent behavior is an integer primary key.
• The majority of users will use integer primary keys throughout their whole application, making it the obvious choice for the more suitable default.

In the end it is the decision of the maintainer. He did announce the change in the changelog, although I have to admit that it should have been a minor release as it may break BC as in your case.

[specialtactics]

@Namoshek The fact that it's "default" makes no difference, as both are equally supported functionality, and it will work for both without casting.

You can not go making assumptions what "the majority" of users are using, and it is not relevant anyway - so long as it's a core feature of Laravel, this package shouldn't break that.

[anteriovieira]

Hi @specialtactics

There are conflicts in this pull request, if you are still interested in contributing please submit another pull request.

Thanks for the contribution

[specialtactics]

Not sure why you closed this @anteriovieira I've been waiting for someone from the package maintainers to review this for the longest time, if you just let me know there is a conflict now, I would have updated it.

I've opened another PR #550