Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: RecordCustomAudit listener #963

Merged
merged 2 commits into from
Feb 3, 2025
Merged

fix: RecordCustomAudit listener #963

merged 2 commits into from
Feb 3, 2025

Conversation

Pr3d4dor
Copy link
Contributor

This pull request fixes this problem:

OwenIt\Auditing\Listeners\RecordCustomAudit::handle(): Argument #1 ($model) must be of type OwenIt\Auditing\Contracts\Auditable, OwenIt\Auditing\Events\AuditCustom given, called in /mnt/c/Users/bine/Documents/mapos-laravel/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php on line 478 

@willpower232
Copy link
Contributor

I'm assuming that fixing these tests also makes sense as well as correcting the listener

@Pr3d4dor
Copy link
Contributor Author

Pr3d4dor commented Aug 19, 2024

@willpower232 There is a strange behavior, maybe there is a bug in the Laravel core.

This does not work:

Event::dispatch(AuditCustom::class, [$this]);

But this works:

Event::dispatch(new AuditCustom($this));

Copy link
Contributor

@willpower232 willpower232 left a comment

Choose a reason for hiding this comment

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

These changes make sense to me, I would presume that the current way used to work at some point but only as an accident of how ambiguous ::dispatch can be.

@erikn69 I think you refactored src/Auditable.php back in #866 so presumably it worked for you then but as noted in the linked issue, isn't how laravel intended things to be dispatched so are you happy with these changes?

@erikn69
Copy link
Contributor

erikn69 commented Feb 3, 2025

@willpower232 in #866 just unify the repeated code and small changes,
if you check it, Event::dispatch(AuditCustom::class, [$this]); was always present, and it works in tests

When the given "event" is actually an object we will assume it is an event object and use the class as the event name and this event itself as the payload to the handler, which makes object based events quite simple.
laravel/framework/src/Illuminate/Events/Dispatcher.php#L244-L268

The Laravel code specifies that the same event object is sent to the handler, only if the first argument is an object,
That makes me think that something out of the ordinary is happening in that scenario, I can't reproduce it,

I'm not happy changing the signature, but it is the most feasible solution that I see

@erikn69 erikn69 merged commit 40fbbe1 into owen-it:master Feb 3, 2025
17 checks passed
@zoispag
Copy link
Contributor

zoispag commented Feb 18, 2025

This is a breaking change, isn't it?

Many of our tests are now failing with error:

TypeError: OwenIt\Auditing\Listeners\RecordCustomAudit::handle(): Argument #1 ($event) must be of type OwenIt\Auditing\Events\AuditCustom, App\Models\Book given, called in ./vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php on line 479

Changing: Event::dispatch(AuditCustom::class, [$this]); to Event::dispatch(new AuditCustom($this)); fixed the issue.

If this is on purpose, a new major version should be released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants