Allow class conversations

[sergix44]

No description provided.

[sergix44]

@cheeghi Could you take a quick check if maybe I overlooked something? This pr aims to make Zanzara fully OOP friendly, so we can use class definitions for conversations, middleware etc. Missing tests.

[cheeghi]

Hi, thanks for the PR. Would be useful to cover also listener's middleware. Right now are resolved only the global middleware (those applied to every listener callback), but if someone does:

$bot->onCommand('start', function(Context $ctx) {
    // ...
})->middleware([MyMiddleware::class, 'myMethod'];

the middleware class is never resolved by the container.

I think there may be some trouble getting the container inside the MiddlewareCollector, but if you could take a look at this and implement a first draft we'd appreciate.

[sergix44]

Hi, thanks for the PR. Would be useful to cover also listener's middleware. Right now are resolved only the global middleware (those applied to every listener callback), but if someone does:

$bot->onCommand('start', function(Context $ctx) {
    // ...
})->middleware([MyMiddleware::class, 'myMethod'];

the middleware class is never resolved by the container.

I think there may be some trouble getting the container inside the MiddlewareCollector, but if you could take a look at this and implement a first draft we'd appreciate.

This case should be covered by this in the ListenerCollector, right?
https://github.com/badfarm/zanzara/pull/17/files#diff-6dfc44da07393f3e9497271bdfcd835644b66ea8dc785dfd24bcd91ac608c092R443-R452

Basically, resoving the callable before instantiate the MiddlewareCollector

[cheeghi]

That middleware method is used to gather global middleware. Then these global middleware are applied to all listeners through this method. In addition to global middleware the user can declare a middleware specific for a listener, but this is another access point, not covered by the middleware method you linked.

[sergix44]

Latest commit should resolve it. Removed also a reference to the React filesystem, bacause it causes errors since the library is no longer included (i guess because the php 8 support)

[cheeghi]

Ok good, I think the getCallable here now can be removed since it's already solved in MiddlewareCollector, then we can merge it. If you want you can write a unit test, otherwise I will add it on develop. For the errors on React filesystem you had useReactFileSystem(true) enabled? Anyway, I'll have a look at it, yes I made the dependency optional if someone wanted to use it (even if not php 8 compatible)

[sergix44]

Ok good, I think the getCallable here now can be removed since it's already solved in MiddlewareCollector, then we can merge it. If you want you can write a unit test, otherwise I will add it on develop. For the errors on React filesystem you had useReactFileSystem(true) enabled? Anyway, I'll have a look at it, yes I made the dependency optional if someone wanted to use it (even if not php 8 compatible)

If we remove the call there we stop resolving the global one via the container right?
Ye

Yeah, when disabling the react file system was not a problem.

[cheeghi]

Yeah, when disabling the react file system was not a problem.

For how I thought about it, it's right that the application throws an error. If someone is using ReactFilesystem he/she has to add react/filesystem dependency manually (aware that the library is not php 8 compatible)