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

Added ability to specify class/instance methods as callbacks #10

Merged
merged 3 commits into from
Dec 2, 2020
Merged

Added ability to specify class/instance methods as callbacks #10

merged 3 commits into from
Dec 2, 2020

Conversation

sergix44
Copy link
Collaborator

@sergix44 sergix44 commented Dec 2, 2020

Hi!
This PR add the ability to specify commands/callbacks as class+method references: right now the framework supports only closures:

$bot = new Zanzara("YOUR-BOT-TOKEN");

$bot->onCommand('start', function (Context $ctx) {
    $ctx->sendMessage('Hello');
});

$bot->run();

With this PR also this is possible:

$bot = new Zanzara("YOUR-BOT-TOKEN");

$bot->onCommand('start', [StartCommand::class, 'startMethod']);

$bot->run();

@sergix44 sergix44 marked this pull request as ready for review December 2, 2020 21:01
@cheeghi
Copy link
Collaborator

cheeghi commented Dec 2, 2020

Hi! Thank you for the PR.
Cool feature, I took a look at the changes, they seem clean and well tested. Just one thing, I've seen that you changed the hierarchy of Zanzara, ListenerResolver and ListenerCollector, I suppose you made it to have the container instance available in ListenerCollector, but I don't like the idea of a superclass (ListenerResolver) using a field ($listeners) of its subclass (ListenerCollector). Instead, I would keep ListenerResolver extending ListenerCollector and move the $container field in ListenerCollector.
We use a gitflow workflow so I change the target branch to develop.

@cheeghi cheeghi changed the base branch from master to develop December 2, 2020 21:04
@sergix44
Copy link
Collaborator Author

sergix44 commented Dec 2, 2020

Hi! Thank you for the PR.
Cool feature, I took a look at the changes, they seem clean and well tested. Just one thing, I've seen that you changed the hierarchy of Zanzara, ListenerResolver and ListenerCollector, I suppose you made it to have the container instance available in ListenerCollector, but I don't like the idea of a superclass (ListenerResolver) using a field ($listeners) of its subclass (ListenerCollector). Instead, I would keep ListenerResolver extending ListenerCollector and move the $container field in ListenerCollector.
We use a gitflow workflow so I change the target branch to develop.

Done in last commit

@cheeghi cheeghi merged commit 6c0ba80 into badfarm:develop Dec 2, 2020
@cheeghi
Copy link
Collaborator

cheeghi commented Dec 8, 2020

Hi, we are adding php 8 support for zanzara and unit tests for this feature fail under php 8. In php 8 the array [MyType::class, 'myMethod'] is considered callable only if 'myMethod' is static, otherwise you must provide an array [$myTypeInstance, 'myMethod'] where the method can be non-static. I think it's a bit ugly delegating the class instantiation to the user. We may get rid of the is_callable check (since later the call actually works because we instantiate the class through the container) or we may improve the check. Can you have a look at it? Thanks! (you can fork the feature/php-8 branch).

Edit: I didn't notice last time: the testWithClass method defines two listeners for the start command, but actually zanzara doesn't support (at the moment, may be useful to implement in the future) multiple listeners for a specific command. So only the assertions of $bot->onCommand('start', [TestCommandClassNoConstructor::class, 'start']); are performed, while those of $bot->onCommand('start', [TestCommandClassWithConstructor::class, 'start']); are ignored. You may split the test in two methods.

@sergix44
Copy link
Collaborator Author

sergix44 commented Dec 8, 2020

It should be ok, just check if it is a callable first (so static::class + static method), if this is not a callable then if the object instantiated via DI + method is that a callable, if none of these conditions are true then it probably was not defined correctly. Or ye, get rid of the check entirely.

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.

None yet

2 participants