Skip to content
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@
"require-dev": {
"phpunit/phpunit": "^7.5",
"mockery/mockery": "^1.0",
"orchestra/testbench": "^3.8"
"orchestra/testbench": "^3.8",
"ramsey/uuid": "^3.0"
},
"autoload": {
"psr-4": {
Expand Down
2 changes: 1 addition & 1 deletion src/Models/Audit.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ class Audit extends Model implements \OwenIt\Auditing\Contracts\Audit
protected $casts = [
'old_values' => 'json',
'new_values' => 'json',
'auditable_id' => 'integer',
// Note: Please do not add 'auditable_id' in here, as it will break non-integer PK models
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@specialtactics specialtactics Mar 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link

@Namoshek Namoshek Mar 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/*
|--------------------------------------------------------------------------
| 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. 👍

Copy link
Contributor Author

@specialtactics specialtactics Mar 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

];
}
44 changes: 44 additions & 0 deletions tests/Models/ApiModel.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php

namespace OwenIt\Auditing\Tests\Models;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\SoftDeletes;
use OwenIt\Auditing\Contracts\Auditable;

class ApiModel extends Model implements Auditable
{
use \OwenIt\Auditing\Auditable;
use SoftDeletes;

/**
* @var string UUID key
*/
public $primaryKey = 'api_model_id';

/**
* @var bool Set to false for UUID keys
*/
public $incrementing = false;

/**
* @var string Set to string for UUID keys
*/
protected $keyType = 'string';

/**
* {@inheritdoc}
*/
protected $dates = [
'published_at',
];

/**
* {@inheritdoc}
*/
protected $fillable = [
'api_model_id',
'content',
'published_at',
];
}
28 changes: 27 additions & 1 deletion tests/Unit/AuditableTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use OwenIt\Auditing\Models\Audit;
use OwenIt\Auditing\Redactors\LeftRedactor;
use OwenIt\Auditing\Redactors\RightRedactor;
use OwenIt\Auditing\Tests\Models\ApiModel;
use OwenIt\Auditing\Tests\Models\Article;
use OwenIt\Auditing\Tests\Models\User;
use ReflectionClass;
Expand Down Expand Up @@ -978,9 +979,16 @@ public function itTransitionsWhenTheAuditAuditableIdTypeDoesNotMatchTheModelIdTy
{
$model = factory(Article::class)->create();

// Key depends on type
if ($model->getKeyType() == 'string') {
$key = (string) $model->id;
} else {
$key = (int) $model->id;
}

$audit = factory(Audit::class)->create([
'auditable_type' => Article::class,
'auditable_id' => (string) $model->id,
'auditable_id' => $key,
]);

$this->assertInstanceOf(Auditable::class, $model->transitionTo($audit));
Expand Down Expand Up @@ -1093,6 +1101,24 @@ public function itTransitionsToAnotherModelState(
$this->assertSame($newExpectation, $models[1]->getDirty());
}

/**
* @test
*/
public function itWorksWithStringKeyModels()
{
$model = factory(ApiModel::class)->create();
$model->save();
$model->refresh();

$this->assertCount(1, $model->audits);

$model->content = 'Something else';
$model->save();
$model->refresh();

$this->assertCount(2, $model->audits);
}

/**
* @return array
*/
Expand Down
20 changes: 20 additions & 0 deletions tests/database/factories/ApiModelFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

use Faker\Generator as Faker;
use OwenIt\Auditing\Tests\Models\ApiModel;
use Ramsey\Uuid\Uuid;

/*
|--------------------------------------------------------------------------
| APIModel Factories
|--------------------------------------------------------------------------
|
*/

$factory->define(ApiModel::class, function (Faker $faker) {
return [
'api_model_id' => Uuid::uuid4(),
'content' => $faker->unique()->paragraph(6),
'published_at' => null,
];
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

class CreateApiModelsTestTable extends Migration
{
/**
* Run the migrations.
*
* @return void
*/
public function up()
{
Schema::create('api_models', function (Blueprint $table) {
$table->uuid('api_model_id');
$table->text('content');
$table->timestamp('published_at')->nullable();
$table->timestamps();
$table->softDeletes();
});
}

/**
* Reverse the migrations.
*
* @return void
*/
public function down()
{
Schema::drop('api_models');
}
}