nuwber / rabbitevents

Nuwber's RabbitEvents provides a simple observer implementation, allowing you to publishing and handling for various events that occur in your applications. For example, if you need to react to some event occurred in another API.
MIT License
121 stars 36 forks source link

Fix middleware call and exception #53

Closed chris-murat closed 4 years ago

chris-murat commented 4 years ago

Middleware for wildcard Listener was called using splat operator from payload array. This behavior break the payload structure, so now the payload is passed as is.

By using Laravel/Lumen 7, FatalThrowableError was raised if the handle method returned false. As this is deprecated since Symfony 4.4, it is now replaced by FatalError from symfony/errorhandler.

chris-murat commented 4 years ago

Actually, when writing the test I realize that my previous commits were not fully functional, particularly for standard listener. So I made another fix to Dispatcher class for this.

As requested, I fix the broken test and I also add some tests to check the array structure.

masterjus commented 4 years ago

do your tests describes the problem? I'll try to fix it in other way

chris-murat commented 4 years ago

Okay, you're right that's not the best solution. I think that I understand what you mean and as I am trying to improve my testing skill, let me work on this again.

chris-murat commented 4 years ago

This time, I just focus on the problem, and I think the solution is way more simple. I describe the test as requested.

masterjus commented 4 years ago

Check my commit. Unfortunately, last change ruins listeners call.

Correct me if something wrong in the test

chris-murat commented 4 years ago

Your test is correct, and thankfully it describes the problem I was trying to resolve yesterday.

Actually, the simple listener call with associative array was also breaking the array structure.

So, I made the change by separating middleware payload affectation, and by checking the array before a simple listener call.

masterjus commented 4 years ago

You describing the problem I've solved in new Publisher and with event classes. If you'll use them everything should be ok.

Please try.

It would work because a message made by publisher will always be made not as ['first' => '1'] but as [['first' => '1']]

chris-murat commented 4 years ago

I am already using Event class to publish my message, and indeed the data return by method toPublish was not wrap with [ ].

Actually, it works perfectly for consumers using simple listeners. But in my case as I am using wildcard listeners, $payload argument received by the listener handle method is wrapped but $payload argument received by the middleware method is not.

So, I can deal with it but this behavior is a bit inconsistent or misleading, as simple listener receive splatted payload for arguments, and the wildcard listener receive the full payload.

Maybe, an explanation about this should be added to the doc. Otherwise, the method Dispatcher@createClassListener should be overriding to match the same behavior used for middleware call. What do you think ?

masterjus commented 4 years ago

It should be fixed but in more elegant way :) Tests are there, so it easy to fix now. let me time

masterjus commented 4 years ago

@chris-murat check now

I've added check to Publisher as well. Could you please test this as well?

chris-murat commented 4 years ago

Works perfectly now. However, just to note that the middleware test is not asserting the middleware handle method but the listener one. And for optimization, new conditional block in Dispatcher@makeListener could be move out of the foreach loop.

Concerning the Publisher, maybe a check could also be added for the event class case.

masterjus commented 4 years ago

Ok do it :) This it your PR

masterjus commented 4 years ago

However, just to note that the middleware test is not asserting the middleware handle method but the listener one.

Sorry, please fix it

chris-murat commented 4 years ago

It should be ok now ;)