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

Allow Handler::acceptsTypes() to detect type-hinted interfaces. #5

Closed
wants to merge 3 commits into from
Closed

Allow Handler::acceptsTypes() to detect type-hinted interfaces. #5

wants to merge 3 commits into from

Conversation

DanielCoulbourne
Copy link

I was unable to make the existing tests fail, so I added my own failing test. It's called FailingTest

Essentially, acceptsTypes was explicitly checking whether the queried type had the same name as any of the type hinted params.

This meant that in the case where an interface was typehinted, we could not use Handler::acceptsTypes([ClassThatImplementsInterface::class]) to detect that method.

My fix uses class_implements ( docs here ) to explicitly check whether the type implements the class.

I added a global feature flag so that I could test both the working and failing case. Sorry if this is annoying, I'll also open a standalone PR without the feature flag.

@brendt
Copy link
Contributor

brendt commented Aug 24, 2023

It's been a while since I've looked at this codebase, but I remember interfaces being supported? https://github.com/spatie/better-types/blob/main/src/Type.php#L91-L103

In any case, adding a global feature flag isn't the way to go, if there is a bug, I think we should fix it in the block I linked at before.

@sebastiandedeyne
Copy link
Member

I tested this within our event sourcing package and it appears hinting interfaces works fine in projectors, but don't work in aggregate roots.

Digging through the event sourcing package's source code, this is how methods are called in aggregate roots:

Handlers::new($this)
  ->public()
  ->protected()
  ->reject(fn (Method $method) => in_array($method->getName(), ['handleCommand', 'recordThat', 'apply', 'tap']))
  ->acceptsTypes([$event::class])
  ->all()
  ->each(fn (Method $method) => $this->{$method->getName()}($event));

And this is how they're called in projectors & reactors:

Handlers::new($this)
  ->public()
  ->protected()
  ->accepts($event)
  ->all()
  ->each(function (Method $method) use ($event) {
    return $this->{$method->getName()}($event);
  });

I don't know this package well enough to determine how acceptsTypes should or should not behave. But I do think the event sourcing package should be consistent in how it discovers handlers, so I opened a PR (spatie/laravel-event-sourcing#434) to normalize the behavior.

@kathunk kathunk closed this by deleting the head repository Oct 28, 2023
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