Skip to content

[Notifier] Escape . char for Telegram transport #41600

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

Merged
merged 1 commit into from
Jun 10, 2021

Conversation

glukose
Copy link

@glukose glukose commented Jun 8, 2021

Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

Hello,

The MarkdownV2 parse mode of Telegram has the '.' char as a reserved char when sending a message.
It should be escaped but only on this mode because it is not reserved on the others.

Link to the original report in the documentation : symfony/symfony-docs#15420

I tried writing a test but couldn't reproduce the error, it does work on a small project however.

This is my first PR on Sf, hope I did it correctly.

Cheers

@glukose glukose requested a review from OskarStark as a code owner June 8, 2021 07:14
@carsonbot carsonbot added this to the 5.3 milestone Jun 8, 2021
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot carsonbot changed the title [NotifierBridge] Escaped '.' char for telegram transport [Notifier] [NotifierBridge] Escaped '.' char for telegram transport Jun 8, 2021
@OskarStark OskarStark changed the title [Notifier] [NotifierBridge] Escaped '.' char for telegram transport [Notifier] Escaped . char for Telegram transport Jun 8, 2021
@@ -79,8 +79,9 @@ protected function doSend(MessageInterface $message): SentMessage

$options['text'] = $message->getSubject();

if (!isset($options['parse_mode'])) {
if (!isset($options['parse_mode']) || $options['parse_mode'] == TelegramOptions::PARSE_MODE_MARKDOWN_V2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!isset($options['parse_mode']) || $options['parse_mode'] == TelegramOptions::PARSE_MODE_MARKDOWN_V2) {
if (!isset($options['parse_mode']) || $options['parse_mode'] === TelegramOptions::PARSE_MODE_MARKDOWN_V2) {

@OskarStark
Copy link
Contributor

Can you please add this test to the TelegramTransportTest:

    public function testSendWithMarkdownShouldEscapeDots()
    {
        $response = $this->createMock(ResponseInterface::class);
        $response->expects($this->exactly(2))
            ->method('getStatusCode')
            ->willReturn(200);

        $content = <<<JSON
            {
                "ok": true,
                "result": {
                    "message_id": 1,
                    "from": {
                        "id": 12345678,
                        "first_name": "YourBot",
                        "username": "YourBot"
                    },
                    "chat": {
                        "id": 1234567890,
                        "first_name": "John",
                        "last_name": "Doe",
                        "username": "JohnDoe",
                        "type": "private"
                    },
                    "date": 1459958199,
                    "text": "Hello from Bot!"
                }
            }
JSON;

        $response->expects($this->once())
            ->method('getContent')
            ->willReturn($content)
        ;

        $expectedBody = [
            'chat_id' => 'testChannel',
            'text' => 'I contain a \.',
            'parse_mode' => 'MarkdownV2',
        ];

        $client = new MockHttpClient(function (string $method, string $url, array $options = []) use ($response, $expectedBody): ResponseInterface {
            $this->assertSame($expectedBody, json_decode($options['body'], true));

            return $response;
        });

        $transport = $this->createTransport($client, 'testChannel');

        $transport->send(new ChatMessage('I contain a .'));
    }

Not sure its working, but lets see 😄

@OskarStark OskarStark changed the title [Notifier] Escaped . char for Telegram transport [Notifier] Escape . char for Telegram transport Jun 8, 2021
@glukose glukose force-pushed the notifier_telegram_escape branch from 87eaa3a to 3306bdf Compare June 8, 2021 07:37
@glukose
Copy link
Author

glukose commented Jun 8, 2021

Thanks for the test and review @OskarStark, it seems to pass well

@OskarStark
Copy link
Contributor

Don't worry about fabbot.io, its a false positive

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job Clément! A very nice first contribution!

@derrabus derrabus force-pushed the notifier_telegram_escape branch from 3306bdf to 0aa0fcb Compare June 10, 2021 19:23
@derrabus
Copy link
Member

Thank you @glukose.

@derrabus derrabus merged commit e086194 into symfony:5.3 Jun 10, 2021
@fabpot fabpot mentioned this pull request Jun 17, 2021
@faizanakram99
Copy link
Contributor

I must add, not just dot (.), there are a few more characters which must be escaped with MarkdownV2 mode.

From https://core.telegram.org/bots/api#markdownv2-style

In all other places characters '_', '*', '[', ']', '(', ')', '~', '`', '>', '#', '+', '-', '=', '|', '{', '}', '.', '!' must be escaped with the preceding character ''.

@chess-server
Copy link

chess-server commented Jun 25, 2021

Unfortunately, this change breaks the application, when using HTML style (cf https://core.telegram.org/bots/api#html-style) ilo MarkdownV2 for the messages. In HTML-Stlye, the characters mentioned by faizanakram99 are permitted. Escaping them leaves the backslash in the message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants