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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed URL decection #984

Merged
merged 6 commits into from Aug 4, 2021
Merged

Fixed URL decection #984

merged 6 commits into from Aug 4, 2021

Conversation

Histmy
Copy link
Contributor

@Histmy Histmy commented Aug 1, 2021

Hello, I noticed two bugs in URL detection and decided to fix them.

The first one was causing validateURL to be true with links like: https://youtubedcom/v/SBjQ9tuuTJQ or http://www.youtube com/embed/SBjQ9tuuTJQ. (Basically allowing any character between "youtube" and "com".)

And the second one made getURLVideoID return different id than YouTube would use in links like: https://youtube.com/v/SBjQ9tuuTJQ/dQw4w9WgXcQ or https://youtube.com/SBjQ9tuuTJQ/somethink/somethink/dQw4w9WgXcQ. (Basically YouTube uses the first string after /v/ and ytdl was using the one after last /.)

I hope I made everything clear and actually helped instead of causing some problems. 馃榾

@redbrain
Copy link
Contributor

redbrain commented Aug 1, 2021

Thanks for the PR! Good find on the first fix.
Second fix seems like an extreme edge case; is there a scenario in which this is needed?

@Histmy
Copy link
Contributor Author

Histmy commented Aug 1, 2021

Well, I cannot really think of any where it would break something, but it makes it behave like YouTube or Discord embeds would be handling it.

@redbrain
Copy link
Contributor

redbrain commented Aug 2, 2021

Well then, the PR looks fine to merge.

@TimeForANinja
Copy link
Collaborator

would be cool if you could add test cases for these 2 scenarios so that we don't break it again in the future
should be pretty straight forward

else i can do it 馃槈

@Histmy
Copy link
Contributor Author

Histmy commented Aug 4, 2021

I think I added the tests, and hopefully correctly. I also fixed bug I introduced by the first commit. Check if everything is OK 馃榾

@TimeForANinja
Copy link
Collaborator

your new version failed with links like youtube.com/embed/VALID_ID
switched to simply check for the hostname
e.g. youtu.be/VALID_ID
and youtube.com/some word/VALID_ID/whatever
tests are passing - linting is passing - code looks fine to me
would merge if fine with you?

@Histmy
Copy link
Contributor Author

Histmy commented Aug 4, 2021

Sure. I am glad I could at least help with this.

@TimeForANinja TimeForANinja merged commit d785dc9 into fent:master Aug 4, 2021
@TimeForANinja
Copy link
Collaborator

nice 馃憤
thanks for the pr 馃槈

@github-actions
Copy link

github-actions bot commented Aug 4, 2021

馃帀 This PR is included in version 4.9.1 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

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

Successfully merging this pull request may close these issues.

None yet

3 participants