Added deactivate function to DialogManager #28
Conversation
lptn
left a comment
There was a problem hiding this comment.
looks good to me. Please take a look to my suggestion and we can merge it
src/DialogManager.php
Outdated
| public function deactivate(Update $update): void | ||
| { | ||
| $dialog = $this->getDialogInstance($update); | ||
| if(isset($dialog)){ |
There was a problem hiding this comment.
| if(isset($dialog)){ | |
| if ($dialog instanceof Dialog) { |
according best practices, the only one valid usage of isset is to check whether a variable exist in the context.
src/DialogManager.php
Outdated
| /** | ||
| * @api Deactivate existing Dialog. | ||
| */ | ||
| public function deactivate(Update $update): void |
There was a problem hiding this comment.
on second thought I compared signatures of these methods:
public function activate(Dialog $dialog): void;
public function deactivate(Update $update): void;activate is straightforward, you explicitly says which dialog to activate.
deactivate in it's current implementation forgets the active dialog state. These 2 are different conceptions: open dialog and active dialog.
- open: any dialog that started but not finished yet
- active: current open dialog
The packages doesn't have of limitations of a number of open dialogs.
I think such method implementation mixes these conceptions and that will lead to confusion and even bugs
…State and validation of dialog instance changed
|
Added discussed updates to PR |
Added public function "deactivate" to deactivate any existing dialog from outside. This will be useful in case when user inputs command with priority higher, than dialogs, and any dialog should be interrupted. The most popular case is /start command.