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

Version 7.3.7 tries to apply events to variadic and untyped functions in the aggregate root #437

Closed
27pchrisl opened this issue Aug 31, 2023 · 5 comments · Fixed by #438
Closed

Comments

@27pchrisl
Copy link
Contributor

27pchrisl commented Aug 31, 2023

Hi,

The change made in #434 appears to be causing the apply() method to call any methods with variadic or mixed/non-declared types on the aggregate root, as it believes they can handle the event.

        Handlers::new($this)
            ->public()
            ->protected()
            ->reject(fn (Method $method) => in_array($method->getName(), ['handleCommand', 'recordThat', 'apply', 'tap']))
            ->accepts($event) // Captures anything in the aggregate root that can receive this type
            ->all()
            ->each(function (Method $method) use ($event) {
                return $this->{$method->getName()}($event);
            });

I found this because I was using mocks for aggregate roots, and mockery adds a lot of these types of methods. I quickly wrote this which implements a rejection for method that have a variadic parameter, no type, or the type isn't either an interface or is_a the event.

If this is along the right lines I'm happy to tidy it up into a PR?

        Handlers::new($this)
            ->public()
            ->protected()
            ->reject(fn (Method $method) => in_array($method->getName(), ['handleCommand', 'recordThat', 'apply', 'tap']))
            ->reject(fn (Method $method) => collect(
                (new \ReflectionClass($this))->getMethod($method->getName())->getParameters())
                ->filter(
                    fn (\ReflectionParameter $parameter) => !$parameter->isVariadic() && $parameter->getType() && (interface_exists($parameter->getType()->getName()) || is_a($event, $parameter->getType()->getName(), true))
                )
                ->isEmpty())
            ->all()
            ->each(function (Method $method) use ($event) {
                return $this->{$method->getName()}($event);
            });
@27pchrisl
Copy link
Contributor Author

#436 may be related

@fefo-p
Copy link

fefo-p commented Aug 31, 2023

Having implemented the change you suggest, the error now appears like the following:

Spatie\EventSourcing\AggregateRoots\AggregateRoot::Spatie\EventSourcing\AggregateRoots\{closure}(): Argument #1 ($parameter) must be of type Spatie\EventSourcing\AggregateRoots\ReflectionParameter, ReflectionParameter given

However, I cannot find the class Spatie\EventSourcing\AggregateRoots\ReflectionParameter.

Tried to modify the type of $parameter to it and obviously it didn't work. Tried searching for ReflectionParameter class and it only appears as a PHPStorm App class /Applications/PhpStorm.app/Contents/plugins/php-impl/lib/php.jar!/stubs/Reflection

@27pchrisl
Copy link
Contributor Author

Hi @fefo-p, those classes are built-in to PHP, I added backslashes to the example so that should work for you now.

@fefo-p
Copy link

fefo-p commented Aug 31, 2023

Hi @fefo-p, those classes are built-in to PHP, I added backslashes to the example so that should work for you now.

will try it again. Thx for the follow-up

@fefo-p
Copy link

fefo-p commented Aug 31, 2023

Apparently it works fine now.
Will have to make more tests though, but now events have been recorded and written to DB.
Thanks @27pchrisl!

I hope this patch can make it to main/master branch!

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