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

Fix Firefox regex to handle version without patch segment #530

Merged
merged 4 commits into from
May 20, 2023

Conversation

karissekjw
Copy link
Contributor

@karissekjw karissekjw commented Oct 26, 2022

Update regex to allow patch segment as optional.

Current Issue

When parsing 'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:104.0) Gecko/20100101 Firefox/104.0' to https://github.com/ua-parser/uap-ruby, #<MatchData "Firefox/103.0" 1:"Firefox" 2:"104" 3:"0" 4:""> is returned which interpolates to Firefox 104.0. (Notice the additional . here due to the empty string in the patch segment)

Solution

With the updated regex, patch segment is optional such that it returns Firefox 104.0 if it's not provided

regexes.yaml Outdated
@@ -921,7 +921,7 @@ user_agent_parsers:
# AFTER THE EDGE CASES ABOVE!
# AFTER IE11
# BEFORE all other IE
- regex: '(Firefox)/(\d+)\.(\d+)\.(\d+)'
- regex: '(Firefox)/(\d+)\.(\d+)(?:\.(\d+)|\z)'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if the \z assertion is supported by every regex-engine. JavaScript seams to not support (or ignore) it. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions/Assertions
Can you please change that to '(Firefox)/(\d+)\.(\d+)(?:\.(\d+)|)'?

Copy link
Contributor Author

@karissekjw karissekjw Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks for getting back, you're right. However, the above doesn't seem to work as intended for Ruby. Specifically, the version for Firefox/4.0b9pre will result in 4.0 instead of 4.0b9pre

I've updated to (Firefox)/(\d+)\.(\d+)(?:\.(\d+)|$) instead, $ seems to have the same effect as \z (see ref). It's also a common token used and is supported by Javascript. Let me know if this change sounds good to you too.

@karissekjw karissekjw requested a review from commenthol January 11, 2023 08:07
@commenthol commenthol merged commit 13e2d1d into ua-parser:master May 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants