laravel-auditing
laravel-auditing copied to clipboard
owen-it
Audits on custom pivot models broken in latest release
| 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.
Solved it. Needed to add the pivot tables primary key to ->withPivot(...) field list on the two many-to-many models
@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.
@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?
@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.
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?
Also came across this problem. Happens when using detach() or sync() which removes pivot entries.
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;
}
}
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.)
The Detach issue still exists... the package really feels abandoned with the broken documentation
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
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);
$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.
