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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Podcasting uas #7481

Merged
merged 20 commits into from
Nov 7, 2023
Merged

Podcasting uas #7481

merged 20 commits into from
Nov 7, 2023

Conversation

mindreader
Copy link
Contributor

Description:

I work in the podcasting space (SiriusXM, Simplecast) and we've been using matomo's device detector for a long time, and it is time we contributed back.

The podcasting industry mainly uses a user agent list from opawg https://github.com/opawg/user-agents-v2.git, so most of these user agents come from that list.

However I've removed all bots, because the list conflates "bot" with "someone we shouldn't serve ads to".

I've also removed all patterns for which I could not find a real example for in the year or so out of 9.5m unique user agents. So every user agent added here has at least one download, though most are at least a few hundred or more, and I added a test for most variants.

The biggest thing for us is Apple Podcasts. It used to be identified in this library as some mobile app "Podcasts", but I can assure you that is Apple Podcasts. They also translated their user agent into various languages for whatever reason, so all the variants are covered now.

I've hand checked every regex against real user agents. The line between feed reader, mobile app, and mediaplayer is sometimes blurry, and podcasting apps all have samey bad names, so it can get confusing, but I've done my best.

The only other oddity worth note is that AppleCoreMedia/1.0.0 (always 1.0.0) is a very common user agent and it is often picked up as Mobile Safari. As far as I can tell, this user agent merely implies you used a particular library on iOS, and that library supposedly didn't let you set a user agent for many, many years, so there are a ton of apps that identify themselves this way, but they aren't safari, so far as I can tell, so I amended that browser to not match in that case, and added it as a library. We identify it as "iOS Application", as that is the only thing we can think of that seems appropriate.

And as an FYI: I ported this library entirely to rust with all passing tests here: https://github.com/simplecastapps/rust-device-detector/ in case that interests anyone. It is just really hard for our business units to run php in production, so rust is a better fit for us. But I intend to take all upstream changes and encourage people to contribute non code fixes here instead of there.

Thank you!

Review

@liviuconcioiu
Copy link
Collaborator

@mindreader you should remove the changes made to README.md, because this file is updated automatically each week (#7482).

Also, feel free to add the link to your port in README.md, under https://github.com/matomo-org/device-detector#device-detector-for-other-languages

@mindreader
Copy link
Contributor Author

@liviuconcioiu Feedback addressed. Thanks!

@mindreader
Copy link
Contributor Author

mindreader commented Oct 16, 2023

Okay, aside from the change of NYTimes to The New York Times, I have addressed all feedback. Edit: Addressed that as well.

@sanchezzzhak
Copy link
Collaborator

@mindreader pls resolve conflict for regexes/client/feed_readers.yml

@Simbiat
Copy link
Contributor

Simbiat commented Oct 30, 2023

@mindreader just for reference, the conflict is caused by #7490

@mindreader
Copy link
Contributor Author

@Simbiat Should be up to date with master now.

@sgiehl
Copy link
Member

sgiehl commented Nov 6, 2023

@sanchezzzhak any reason you hadn't merged that one?

@sanchezzzhak
Copy link
Collaborator

Hi, everything looks great here. I wanted to give time to the other participants, in case I missed something.

@sanchezzzhak sanchezzzhak merged commit f8b928d into matomo-org:master Nov 7, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants