-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Rework youtube link parsing #18979
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
base: master
Are you sure you want to change the base?
Rework youtube link parsing #18979
Conversation
1a7e284 to
a31bd2a
Compare
a31bd2a to
63cf0a7
Compare
|
Could you add a test for the url structure mentioned in #18780 to ensure it doesn't try to embed? And then would you mind providing a copy+paste-able block of sample URLs that we can put in a blog or forum post to test the behavior locally? |
63cf0a7 to
d8c63eb
Compare
|
Links to test with (rick roll not intended)
|
bf31ede to
3c70812
Compare
|
LGTM |
|
we can check also: |
Hmm, there is an edge-case, if user sends video URL like https://www.youtube.com/watch?v=Nf7oA1Q42Pw&list=PLe5ZNOR8Ttm1HHLAo64Hv4XGa_XU5-a7t&index=1 It will be parsed as regular watch, instead of playlist + watch. Should we handle that ? |
* Add unit tests * Don't use complex lont regexes
66ea354 to
9babddb
Compare
|
|
||
| private def addProtocolIfNecessary(url: String): String = | ||
| if url.matches("(?i)^https?://.*") then url | ||
| else s"http://$url" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should be https here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I did it like this initially, but thought what if the server is http only. If its https and correctly configured it will HTTP 301 to the secure url. We don't know what link they will paste in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 2025 http:// is fully dead, I haven't seen such a link for a long time.
- Rename variable
What I aim to do with this PR:
youtu.beandyoutube.comURLs, which are structurally different.www.youtube.com) the link goes to e.g.https://lichess.org/forum/general-chess-discussion/training-your-chess-intuition-and-board-awareness/www.youtube.comhttps://www.youtube.comwherehttp://is added by default, if URL protocol is missing.Closes #18780