Audits on custom pivot models broken in latest release

[amsoell]

Q	A
Bug?	yes
New Feature?	no
Framework	Laravel
Framework version	6.4.0
Package version	9.3.2
PHP version	7.2.21

Actual Behaviour

When auditing custom pivot models that have autoincrementing IDs, the audit event errors with a Column 'auditable_id' cannot be null error

Expected Behaviour

Pivot model data saved to audits table

Steps to Reproduce

On a clean Laravel 6.4 application, create two models related with a many-to-many relationship, using a custom intermediate table model that has an auto-incrementing primary key. With version 9.3.2 of the Laravel-Auditing package, any changes in the relationship will result in an error about the auditable_id value being null. Downgrading to version 9.3.1 doesn't see this error.

Possible Solutions

Version 9.3.2 was an intended bugfix for non-integer primary keys on audited models, I suggest re-evaluating that pull request. I think this could be worked-around by extending the Audit model and re-adding the auditable_id cast, but that would re-break whatever PR #550 was intended to fix.

[chosten]

I have that error even in version 9.3.1

[ArtDanFree]

any news on this issue?

[DSpeichert]

This is affecting my app in v10.0 of this app with newest Laravel 7.

[diego-lipinski-de-castro]

Is this package abandoned?

[Miguel-Serejo]

Does anyone have a workaround for this?

[AharonFeinstein]

also have this problem

[AharonFeinstein]

Solved it. Needed to add the pivot tables primary key to ->withPivot(...) field list on the two many-to-many models

[sudeerax]

@AharonFeinstein Can you provide some code?

[AharonFeinstein]

@sudeerax My pivot table has an incrementing primary key id,

public function students()
    {
        return $this->belongsToMany(Student::class)
                    ->using(CourseStudent::class)
                    ->withPivot([
                        'id',
                        'percent_grade',
                        'date',
                        'sales_person_id',
                        'sales_value',
                    ])
}

When I added id to the list of fields in withPivot, on both the courses model, and the students model, the issue went away. I also have public $incrementing = true; on the CourseStudent pivot model but I dont think that made a difference.

[aanderse]

@AharonFeinstein thanks for the tip. This works well for me when I call attach, but not when I call detach. It looks like the id attribute isn't included in my pivot model even when I call withPivot([ 'id', ... ]) in the parent model. Can please you confirm if detach works for you or not?

[AharonFeinstein]

@aanderse you're right, it's still broken for detach. I could do App\Student::first()->courses->first()->pivot->delete() which logs the audit data... but thats pretty annoying. Darn.

[aanderse]

After some more research I've concluded that this isn't a bug with laravel-auditing at all. The documentation for laravel-auditing needs to be updated to include the ->using(CustomPivot::class)->withPivot([ 'id', ... ]) part and an issue needs to be filed upstream with laravel. Note that this issue indicates the primary id field should show up, but I do have some concerns laravel made a conscious decision here... When you attach multiple models together and then call detach only one deleting and one deleted event is fired, so this may actually be on purpose (which would be a bad idea in my opinion because pivot tables can hold extra data making them unique).

Does anyone have the motivation to file such an issue with laravel?

[yuri-kovalev]

also have this problem

[liepumartins]

Also came across this problem. Happens when using detach() or sync() which removes pivot entries.

[mo7zayed]

You can override the getKey function that is used by the Auditable trait in your model and make the fallback value to 0

This will store auditable_id to 0 in the database

use OwenIt\Auditing\Contracts\Auditable;
use OwenIt\Auditing\Auditable as AuditableConcrete;

class YourPivotModel extends Pivot implements Auditable
{
    use AuditableConcrete;

    /**
     * Get the value of the model's primary key.
     *
     * @return mixed
     */
    public function getKey()
    {
        return $this->getAttribute($this->getKeyName()) ?? 0;
    }
}

[wunc]

Just adding public $incrementing = true on the pivot model (with an auto-incrementing id column) was enough to get this to work with attach() for me. No need to add the id to the ->withPivot() array.

Still throws an error on detach(), though. I think this is due to the delete event for pivot models not having the id when triggered through detach, which seems to maybe be a Laravel issue. So, the workarounds mentioned previously (either deleting the pivot model instance directly or overriding getKey to return a dummy key) are needed.

edit: Since this issue is specific to writing the audit, a safer alternative workaround to overridding getKey(), which might have unintended consequences, is to use transformAudit(), e.g.:

public function transformAudit(array $data): array
{
    // The detach model event doesn't properly include the pivot model id,
    // so we need this workaround for now.
    $data['auditable_id'] = $data['auditable_id'] ?? 0;

    return $data;
}

(I'm using laravel 8 and laravel-auditing 12.)

[ThaDaVos]

The Detach issue still exists... the package really feels abandoned with the broken documentation

[aymenel]

Hello, i use this trait

trait PivotAuditable
{
    use Auditable;

    public function transformAudit(array $data): array
    {
        $isEventDeleted = Arr::equalValue($data, 'event', 'deleted');
        $isHasNotAuditableId = Arr::equalValue($data, 'auditable_id', NULL);

        if ($isEventDeleted && $isHasNotAuditableId) {
            $where = [];
            foreach (Arr::get($data, 'old_values', []) as $key => $value)
                $where[$key] = $value;

            tap(self::where($where)->get(self::getKeyName()), function($id) use (&$data) {
                if ($id)
                    $data['auditable_id'] = $id;
                });
            }

            return $data;
    }
}

Laravel 8.83.18 Laravel auditing 13.0.3

[aSeriousDeveloper]

A fix / workaround for this is already included in the docs: https://laravel-auditing.com/guide/audit-custom.html#example-related-attributes

Instead of attaching the Auditable classes to your Pivot Tables, you instead need to change your attach() and detatch() methods. You now call the attach() on the original model as auditAttach($relationship), and you don't call the relationship() method on the model.

Old Method:

$user->posts()->detach($post);

New Method:

$user->auditDetach('posts', $post);

[diego-lipinski-de-castro]

@aSeriousDeveloper this still throws error

[lianmaymesi]

$student_pivot_id = $course->student()->where('user_id', $user->id)->first()->pivot->id;

CourseStudent::where('id', $student_pivot_id )->delete();

If you do this, constraint null error not happen again. I have tested and it's working.

[nikspyratos]

This issue seems generally inconsistent between Laravel versions too. I have this problem on a L11 repo whether or not using sync vs auditSync (same for detach), but not on a L10 repo with the same code.

transformAudit as mentioned above is the safest place to do this modification (so that you can still use sync etc and not add more overhead), but for deletion events you still won't be able to do a refresh or a manual query unless you're using soft deletes for the pivot model.

With L11 though, one workaround can be to use Context. Add a deleting event handler in your model to set the context:

protected static function booted(): void
    {
        static::deleting(static function ($model) {
            if (is_null($model->id)) {
                Context::push('audit_pivot_name_id', $model->refresh()->id);
            }
        });
    }

Then in your transformAudit you can pull from Context:

public function transformAudit(array $data): array
    {
        if (is_null($data['auditable_id'])) {
                $data['auditable_id'] = Context::pull('audit_pivot_name_id');
        }
        return $data;
    }

I'd recommend also checking $data['event'] for which event is happening, so that you can handle edge cases with non-delete events that might throw the same issue.