Can't get Audit Users when User Model uses non-default Primary Key

[ghost]

Q	A
Bug?	yes
New Feature?	no
Framework	Laravel
Framework version	5.4.25
Package version	4.0.7
PHP version	7.0.18

Actual Behaviour

It is not possible to get a Model Audit's User if the User Model uses a non-default Primary Key (ie. a Primary Key different than id).

Expected Behaviour

It should be possible :-)

Steps to Reproduce

• Setup Laravel and Auditing.
• Change the App\User model's Primary Key to something else (like some_id) both in the Model (via the protected $primaryKey property) and the database.
• Add an auditable model:

  
  <?php

namespace App;

use Illuminate\Database\Eloquent\Model; use OwenIt\Auditing\Auditable; use OwenIt\Auditing\Contracts\Auditable as AuditableContract;

class SomeModel extends Model implements AuditableContract { use Auditable;

public $timestamps = false;

/**
 * The attributes that are mass assignable.
 *
 * @var array
 */
protected $fillable = [
    'field1', 'field2', 'field3',
];

}

* Add a table for the new model with a migration with:
```php
Schema::create('some_models', function (Blueprint $table) {
        $table->increments('id');
        $table->string('field1');
        $table->string('field2');
        $table->string('field3');
    });

• Run a simple test like:

  public function testAudits()
  {
  $modelFields = [
      'field1' => 'Field '.rand(),
      'field2' => 'mail'.rand().'@mail.com',
      'field3' => 'password'.rand(),
  ];
  
  $model = new SomeModel($modelFields);
  
  // set a proper ID in here
  Auth::onceUsingId(1);
  $model->save();
  
  // get model with audits and users
  $model = SomeModel::with('audits.user')->find($model->getKey());
  
  echo "model: \n";
  print_r($model->attributesToArray());
  
  $audit = $model->audits->first();
  
  echo "audit: \n";
  print_r($audit->attributesToArray());
  
  $user = $audit->user;
  
  echo "user: \n";
  print_r($user->attributesToArray());
  }

Result:

1) Tests\Unit\SomeModelTest::testAudits
Error: Call to a member function attributesToArray() on null

(on the print_r($user->attributesToArray()); line)

Possible Solutions

I got it working by adding a 'user_id' parameter to the Audit::model() method so it looks like:

public function user()
{
    return $this->belongsTo(Config::get('audit.user.model'), 'user_id');
}

[ghost]

I'd be happy to submit a PR if that looks like the correct solution :-)

[quetzyg]

Thanks for reporting the issue @daniel-gomes-sociomantic.

Your proposed solution seems correct, so I'll apply that and update the documentation.

Actually, I just ran a quick test, changing the primary key in the User model from id to user_id and the package still works as expected. Another thing to note is, your proposed solution doesn't make a difference because the second argument will always be resolved to user_id, which is the column in the audits table. In your case, the argument you would need to change is the third one (id column in the users table), but even that one gets resolved correctly when omitted.

You're probably doing something else wrong.

[quetzyg]

@daniel-gomes-sociomantic, any updates, or shall I close this?

[ghost]

sorry haven't had the time to look again into it, will do asap

[ghost]

I've redone all my steps from scratch and still the problem remains :-\

I went digging inside Laravel's belongsTo() method to figure out what's going on with the original package code (ie. without adding the user_id parameter I proposed earlier). In there, when the $foreignKey isn't passed the if (is_null($foreignKey)) block gets activated, which is where I added some echos.

Inside that method, the foreign key gets sets to Str::snake($relation).'_'.$instance->getKeyName(), and:

• With the default User Primary key (id, ie. without setting its Primary Key at all in the User model), we have:
  • Str::snake($relation): user
  • $instance->getKeyName(): id
  • Final Foreign Key: user_id -> works.
• With a custom User Primary key (by setting the User::$primaryKey value to user_id), we have:
  • Str::snake($relation): user
  • $instance->getKeyName(): user_id (note that $instance is a App\User instance).
  • Final Foreign Key: user_user_id -> doesn't work.

As you said, that second parameter ($foreignKey) is the name of the column in the audits table, so it should always evaluate to user_id, which doesn't seem to happen when the User model's Primary Key is not id.

[quetzyg]

Ha! I see what you mean.

The quick test I did was with a Laravel 5.3 installation I already had, which resolves the foreign key with the _id suffix

if (is_null($foreignKey)) {
    $foreignKey = Str::snake($relation).'_id';
}

rather than using the $instance->getKeyName() value.

You'll have to update the audits table user_id column to user_user_id, in order for that to work.

Any particular reason to have a different column name for the id?

[ghost]

Any particular reason to have a different column name for the id?

legacy code (done before the laravel-era at a time when table ID's were not all id)

You'll have to update the audits table user_id column to user_user_id, in order for that to work.

That doesn't make much sense to me: the foreign key, as defined in the package, is named user_id, but unfortunately, as I showed, Laravel makes the foreign key default to something else in certain conditions (apparently starting only in more recent versions). The thing is, the the Auditing package knows the Foreign key is called user_id, so why not define it directly in the package itself, making it work anytime anywhere, instead of leaving that to the programmer?

It's not about laziness or not wanting to do it, it's just that if you'll leave that to the programmers, you'll have to at least warn them (in the documentation) of this fact. Won't that leave the door open to some confusion?

[quetzyg]

the foreign key, as defined in the package, is named user_id

Good point, actually. I'll work on that.

[quetzyg]

Hi @daniel-gomes-sociomantic,

This issue should now be fixed in master. Have a go and let us know if all is OK. I will close this issue once I get positive feedback.

I haven't updated the documentation yet, but the change is very simple.

You'll have to add two new keys (primary_key and foreign_key) to the user configuration. For your specific case, it should be like so:

<?php
return [
    'user' => [
        'primary_key' => 'user_id',
        'foreign_key' => 'user_id',
        'model'       => App\User::class,
        'resolver'    => function () {
            return Auth::check() ? Auth::user()->getAuthIdentifier() : null;
        },
    ],
];

[ghost]

trying it out, but the package hit this (new) problem when trying to store an Audit:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'updated_at' in 'field list' 
(SQL: insert into `audit_new` (`old_values`, `new_values`, `event`, 
`auditable_id`, `auditable_type`, `user_id`, `url`, `ip_address`, `user_agent`, 
`created_at`, `request_id`,  `comment`, `updated_at`) values (...}, created, 65, 
App\User, 1, , , , 2017-07-03 08:42:00, 8sHU2VbL, , 2017-07-03 08:42:00))

Ie. it couldn't insert the updated_at value.

I got it solved by adding:

const UPDATED_AT = null;

To the package's Audit Model.

[quetzyg]

Ah yes, I forgot to mention that the created_at updated_at column is now needed, so you'll have to update the audits table.

Other than that, is the key issue solved?

[ghost]

Ah yes, I forgot to mention that the created_at column is now needed

the problem seems to be with the updated_at column, the Audit's table created_at column was already there and seems fine

[quetzyg]

Yeah, my bad. I edited my previous comment.

[ghost]

right, but why is the Audit's table updated_at column needed if (correct me if I'm wrong) an Audit row is never expected to be updated?

[quetzyg]

The new version allows extending other model classes besides the typical Illuminate\Database\Eloquent\Model, and having the updated_at, means less boilerplate when implementing the OwenIt\Auditing\Contracts\Audit interface.

So you don't need to have:

    /**
     * {@inheritdoc}
     */
    public $timestamps = false;

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

in your implementation.

And while you're correct in stating that an Audit record shouldn't be modified, you can. So at least this way you can check if records have been tampered with, by comparing the created_at and updated_at values.

Make sense?

[ghost]

So you don't need to have: in your implementation.

Actually I'm a bit lost. A project 's auditable model will implement the OwenIt\Auditing\Contracts\Audit interface and may, or may not, have timestamps - but that is for the Model itself to decide and isn't related to the Auditing feature. The problem I was referring to was for the package's audit table rows, and whether they only need the created_at timestamp (as before) or both timestamps (as now) - this is where code like the one you wrote would come into play (in the package's Audit Model).

And while you're correct in stating that an Audit record shouldn't be modified, you can. This way you can check if records have been tampered with, by comparing the created_at and updated_at values.

Couldn't a tamperer simply update the updated_at column to the value of created_at when tampering with other columns? :-)

Anyway, personally I don't see the need for an audit.updated_at column, and my only problem is that it seems there is no way to disable it (short of editing package's code, which is not doable). I guess I'll simply add it and forget about it, it's just that it feels like a useless waste of space since I expect it to always match the value of created_at.

[quetzyg]

Couldn't a tamperer simply update the updated_at column to the value of created_at when tampering with other columns? :-)

Indeed. You can even change the user_id to blame someone else. The package doesn't stop you from doing those types of things. And the space you waste by having the updated_at is pretty much negligible, it's just metadata.

More to the point, is the original issue you had with the User primary key fixed?

[ghost]

Indeed. You can even change the user_id to blame someone else. The package doesn't stop you from doing those types of things. And the space you waste by having the updated_at is pretty much negligible, it's just metadata.

fair enough.

More to the point, is the original issue you had with the User primary key fixed?

Yeah, seems fixed, thanks!

Note sure if you've done that yet, but if the audit table's Foreign Key Column name can be configured by the developers (in audit.php) , then the CreateAuditsTable migration should be adapted to use that column name (same for the primary key, I guess)

[quetzyg]

Good catch! It's done, now.

[MachariaK]

I have a similar situation. I have a non-default primary id which is a VARCHAR and the Audit fails with the following exception:

SQLSTATE[22007]: Invalid datetime format: 1366 Incorrect integer value: 'hotmail.com' for column 'auditable_id' at row 1.

The model primary key is VARCHAR but the 'auditable_id' is BIGINT. I have changed the 'auditable_id' to varchar but I am not sure whether this will affect the package's functionality.