Skip to content

Conversation

markieo1
Copy link

Hi,

As discussed in #1031, hereby the PR.

The code defaults to the ProcessDispatchAudit. When a different implementation is provided or null, Laravel uses this. Laravel already handles the null correctly.

Using it this way anyone can provide a custom listener implementation.

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.

LGTM, @erikn69 did you have an opinion on whether we should either allow this to be overridden or just provide the extra option as mentioned in #1031 ?

Co-authored-by: Will Power <1619102+willpower232@users.noreply.github.com>

Event::listen(AuditCustom::class, RecordCustomAudit::class);
Event::listen(DispatchAudit::class, ProcessDispatchAudit::class);
Event::listen(DispatchAudit::class, Config::string('audit.queue.listener', ProcessDispatchAudit::class));
Copy link
Contributor

Choose a reason for hiding this comment

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

@willpower232 hi, maybe an interface check, to prevent someone from setting up a class that is not a listener, or some non-existent class

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to have settable listeners, shouldn't RecordCustomAudit listener have the same option?

Copy link
Contributor

Choose a reason for hiding this comment

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

@markieo1 what do you think about adding an option for the custom audit and then using class_exists and probably method_exists for handle? not sure there is an interface for event listeners

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a good option, another could be to create a new contract that requires having a handle method and validate that interface.

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