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

Add support for Message Thread ID (Topics) #5

Merged
merged 6 commits into from
Jan 24, 2025

Conversation

Niklan
Copy link
Contributor

@Niklan Niklan commented Jan 23, 2025

This PR adds optional support for sending messages to specific group threads. This is a feature of group chats with the Topics feature enabled.

Parameter Type Required Description
message_thread_id Integer Optional Unique identifier for the target message thread (topic) of the forum; for forum supergroups only

https://core.telegram.org/bots/api#sendmessage

For backward compatibility, it is added as a last parameter.

@jacklul
Copy link
Owner

jacklul commented Jan 23, 2025

Looks good but please fix the error code sniffer is whining about

@Niklan
Copy link
Contributor Author

Niklan commented Jan 24, 2025

Also, it looks like a mistake in \jacklul\MonologTelegramHandler\TelegramHandler::__construct

it says:

@param Level     $level           The minimum logging level at which this handler will be triggered

but actually it can handle not just \Monolog\Level but also int and string: int|string|Level $level = Level::Debug.

This can be a problem with PHPStan or any other static code analyzer, which prioritizes, in most cases, document block parameter definitions over actual type hints. Looks like it should be improved to:

@param int|string|Level $level    The minimum logging level at which this handler will be triggered

If it should be fixed, then it should be done as a separate pull request or we can fix it here?

If you agree that this should be fixed, what a proper formatting (alignment) should be in that case?

Variant №1:

    /**
     * @param string    $token           Telegram bot API token
     * @param int       $chatId          Chat ID to which logs will be sent
     * @param int|string|Level $level    The minimum logging level at which this handler will be triggered
     * @param bool      $bubble          Whether the messages that are handled can bubble up the stack or not
     * @param bool      $useCurl         Whether to use cURL extension when available or not
     * @param int       $timeout         Maximum time to wait for requests to finish
     * @param bool      $verifyPeer      Whether to use SSL certificate verification or not
     * @param int|null  $messageThreadId Thread ID for group chats with Topics feature enabled
     */

Variant №2:

    /**
     * @param string           $token           Telegram bot API token
     * @param int              $chatId          Chat ID to which logs will be sent
     * @param int|string|Level $level           The minimum logging level at which this handler will be triggered
     * @param bool             $bubble          Whether the messages that are handled can bubble up the stack or not
     * @param bool             $useCurl         Whether to use cURL extension when available or not
     * @param int              $timeout         Maximum time to wait for requests to finish
     * @param bool             $verifyPeer      Whether to use SSL certificate verification or not
     * @param int|null         $messageThreadId Thread ID for group chats with Topics feature enabled
     */

Both variants passes composer check-code, so it is unclear what is intended formatting.

@Niklan Niklan changed the title Add support for Topics (Thread ID) Add support for Message Thread ID (Topics) Jan 24, 2025
@jacklul
Copy link
Owner

jacklul commented Jan 24, 2025

I don't mind fixing it in this PR, the second variant is more readable

@Niklan
Copy link
Contributor Author

Niklan commented Jan 24, 2025

Synced with the master and added a second variant of the docblock to \jacklul\MonologTelegramHandler\TelegramHandler::__construct.

@jacklul jacklul merged commit 47c85c4 into jacklul:master Jan 24, 2025
1 check passed
@jacklul
Copy link
Owner

jacklul commented Jan 24, 2025

Thanks!

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.

2 participants