Fix all tests
- [x] I carefully read the contribution guidelines and agree to them.
- [x] I have tested the API against NewPipe.
This PR fixes all test.
It also introduces an option to change the consent.
Supersedes #871
The consent is only required for some playlists (#876) and mixes (#820) in EU.
That's why I'm strongly against allowing tracking in all YouTube requests we made. We should only do so where it is needed.
Remember that one of the main goals of the NewPipe projects is to enhance privacy.
Yes. This PR does not allow any new consent, as it is false by default.
What I see is that the consent variable has been wrapped in a setter/getter. I assume this is so that other NPE consumers can easily show a consent prompt, e.g. Piped?
What I see is that the consent variable has been wrapped in a setter/getter. I assume this is so that other NPE consumers can easily show a consent prompt, e.g. Piped?
- Yes, in theory this should be possible now
- The variable is static so it's technically no getter or setter
- The static variable might change in the future for now it's primarily designed as a workaround so that the tests work again
The variable is static so it's technically no getter or setter
I didn't get this bit.
@Stypox I'm done with my changes you can review again if you have time.
Update: So there have been a bunch of merge conflicts due to the recent hotfix.
These should be fixed now.
While fixing the conflicts I also fixed 2 additional problems:
- YouTube Mixes no longer work for the specified (kurzgesagt) video
- The suggestion/"Did you mean..." now longer works for the specified search term
For https://github.com/TeamNewPipe/NewPipeExtractor/pull/882#issuecomment-1205007648, it seems you don't have to change something, as addYoutubeHeaders() is never called except by YoutubeMixPlaylistExtractor (this method should be called everywhere, don't touch anything here because in order to respect what I said, additional changes would be needed1), so the consent cookie is only sent on mixes.
Note that the current code as it is would not fix #876, as this consent is not applied on regular playlists.
1Here is my idea:
- Change
generateConsentCookie()togenerateConsentCookie(boolean), where the boolean parameter would be something likeuseYesValue. IfuseYesValueis true, returnYES+; otherwise returnPENDING=and the three random digits; - Change
addCookieHeader(@Nonnull final Map<String, List<String>> headers)toaddCookieHeader(@Nonnull final Map<String, List<String>> headers, final boolean consentValue); - Use the boolean value provided to call
generateConsentCookie(boolean); - Change
addYoutubeHeaders(@Nonnull final Map<String, List<String>> headers)toaddYoutubeHeaders(@Nonnull final Map<String, List<String>> headers, final boolean consentValue); - Call
addCookieHeader(headers, consentValue)in this method; - Use
addYoutubeHeaders(headers, YoutubeParsingHelper.isConsentAccepted())in playlists and mixes (addYoutubeHeadersshould be used anywhere, but that's not related to your PR at all); - Note: singleton lists should not be used on
Cookieheader, to allow putting a reCAPTCHA one.
For #882 (comment), it seems you don't have to change something, as
addYoutubeHeaders()is never called except byYoutubeMixPlaylistExtractor(this method should be called everywhere, don't touch anything here because in order to respect what I said, additional changes would be needed1), so the consent cookie is only sent on mixes.Note that the current code as it is would not fix #876, as this consent is not applied on regular playlists.
1Here is my idea:
Should I react to this?
If yes: Your solution doesn't make much sense to me, you can simply call addCookieHeader or not ;)
It also adds a lot of overhead for doing technically the same.
Also with foresighted to https://github.com/TeamNewPipe/NewPipeExtractor/pull/833: This would be an instance specific property and I think it can be handled in the settings of an instance with e.g. a toggle.
Note: I also fixed https://github.com/TeamNewPipe/NewPipeExtractor/issues/902 by
- doing a check if the extracted JS is valid JavaScript and if not try the fallback (Regex)
- Removing a
}from the regex so that it matches correctly
@litetex Could you update the PR description to mention all the changes this makes, such as https://github.com/TeamNewPipe/NewPipeExtractor/pull/882#issuecomment-1221596544?