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

AggregateRoot Method is called if Argument has no type-hint #341

Closed
fabianpnke opened this issue May 7, 2022 · 8 comments
Closed

AggregateRoot Method is called if Argument has no type-hint #341

fabianpnke opened this issue May 7, 2022 · 8 comments
Assignees

Comments

@fabianpnke
Copy link

PHP Version: 8.1.5
Laravel Version: 9.11.0
spatie/laravel-event-sourcing Version: 7.2.0

Hi there,

Today I came across the Situation where Method two was getting called when using the AggregateRoot with Method one inside the Application. I wondered why this was happening cause I had never used the AggregateRoot with Method two anywhere yet. After some experimenting and debugging, I found out that this only happens if the Argument in Method two is not type-hinted.

Here are some examples for a better understanding:

Scenario 1 (two is called):

<?php

class SomethingAggregateRoot extends AggregateRoot
{
    public function one(string $foo): self
    {
        ...
        $this->recordThat(new SampleEvent());
        ...
        return $this;
    }

    public function two($without_typehint): self
    {
        ...
        $this->recordThat(...);
        ...
        return $this;
    }
}

SomethingAggregateRoot::retrieve($uuid)
    ->one('laravel')
    ->persist();
// Result: two was called

Scenario 2 (two is not called):

<?php

class SomethingAggregateRoot extends AggregateRoot
{
    public function one(string $foo): self
    {
        ...
        $this->recordThat(new SampleEvent());
        ...
        return $this;
    }

    public function two(boolean $with_typehint): self
    {
        ...
        $this->recordThat(...);
        ...
        return $this;
    }
}

SomethingAggregateRoot::retrieve($uuid)
    ->one('laravel')
    ->persist();
// Result: two was not called

Is this an expected behaviour or a bug/defect?

Thanks for checking and clarifying,
Fabian

@freekmurze
Copy link
Member

To my knowledge, our packages wouldn't call a custom method like two.
Could you create a minimal Laravel app in which the problem is reproduced?

@fabianpnke
Copy link
Author

Please see https://github.com/fabianpnke/laravel-event-sourcing-issue-341-demo

I've managed to reproduce a behaviour in that the problem results in a 500 Server error by removing a type hint from the method.

@mariolorente
Copy link

Hi,

same here, when there is a public method in an aggregate with non typed params the app totally breaks.

The break occurs exactly inside the apply function of AggregateRoot.php, in the Handlers section.

@mariolorente
Copy link

mixed doesnt work as a type either.

@zackrowe
Copy link
Contributor

zackrowe commented Jun 28, 2022

I've also ran into this bug. I believe the issue is in the AggregateRoot class line 259. The method doesn't get filtered when the event gets passed to the accepts() method since the method argument is defined as mixed.

Possible Fix Maybe a method could be added to Handlers to filter methods that don't take a specific argument type? In this case ShouldBeStored?

Temporary Solution I added a second unused argument to my function to get things working. Not ideal but will work for now until this is addressed.

@zackrowe
Copy link
Contributor

I've opened a PR in the better-types repository that can hopefully help address this issue. spatie/better-types#3

@zackrowe
Copy link
Contributor

zackrowe commented Aug 1, 2022

@freekmurze I've tested the latest changes in version 7.2.3 in my project and I believe this issue has been resolved.

@fabianpnke
Copy link
Author

Hi @zackrowe and @freekmurze
I've bumped the package to the latest version in the example repo I've provided. And now it works!
Thanks @zackrowe for providing a fix for this issue.

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

No branches or pull requests

5 participants