asantibanez commented 6 years ago

Hi. This PR tries to address an issue found with query parameters during the login event. When array parameters are present in the url, the validation crashes.

Overview

The TelegramDriver runs the isValidLoginRequest method on every route hit in a Laravel project I currently have setup with Botman. Botman is setup manually.

NOTE: This may or not be the issue so that is why I am describing my current setup.

routes/botman.php

I recently installed Laravel Nova to take advantage of this new tool and everything worked except when hitting routes that had array parameters in the url. Nova would complain with a Array to string conversion exception thrown by the TelegramDriver which I thought was very weird. This got me digging into the matter and found that the issue was on the isValidLoginRequest when converting the query parameters to a string.

Since some of Nova's query parameters were arrays, this method failed.

PS: I am sorry if this is a matter of my project's setup and really not an issue. I appreciate your time. Thanks for Botman and this driver.

codecov[bot] commented 6 years ago

Codecov Report

Merging #49 into master will increase coverage by 0.31%. The diff coverage is 100%.

@@             Coverage Diff              @@
##             master      #49      +/-   ##
============================================
+ Coverage      76.2%   76.51%   +0.31%     
- Complexity      117      119       +2     
============================================
  Files            11       11              
  Lines           374      379       +5     
============================================
+ Hits            285      290       +5     
  Misses           89       89

Impacted Files	Coverage Δ	Complexity Δ
src/TelegramDriver.php	`93.63% <100%> (+0.2%)`	`46 <1> (+2)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 32ed131...676524c. Read the comment docs.

asantibanez commented 6 years ago

Hi @mpociot. When you are available, could you please check this one out? Thank you very much. Sorry about the trouble.

PS: Thanks for building Botman. It is amazing!

stefandanaita commented 5 years ago

Hello @mpociot. Can you please look into this PR? It's still breaking 😢

asantibanez commented 5 years ago

Hi @stefandanaita. Good to see some others interested in the PR. Is the PR breaking? Do I need to update anything? Or are you having the same issue than me? Thanks in advance.

stefandanaita commented 5 years ago

Hi @asantibanez! Sorry for not being clear. The PR is alright, I'm having the exact same issue as you do.

asantibanez commented 5 years ago

Thanks for replying @stefandanaita. Hopefully we can get this one merged or get feedback from @mpociot . Good to know I'm not the only one with the issue.

filippotoso commented 2 months ago

I'm closing this. Just configure Botman to answer to a specific route and it works perfectly:

https://github.com/botman/botman/discussions/1308#discussioncomment-3008604

This will also improve you application performance because Botman will not process every single request.

botman / driver-telegram

Telegram Login Event - Edge Cases Validation #49

Codecov Report