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

os() function could be improved #15

Closed
dave opened this issue Nov 12, 2014 · 7 comments
Closed

os() function could be improved #15

dave opened this issue Nov 12, 2014 · 7 comments

Comments

@dave
Copy link

dave commented Nov 12, 2014

The os() function is a bit broken... It simply cycles through the oss rules, and returns the key of the first matching rule. If it's going to do this, the rules should be carefully re-ordered. For example, this is the user agent for Windows Phone 8.1:

Mozilla/5.0 (Mobile; Windows Phone 8.1; Android 4.0; ARM; Trident/7.0; Touch; rv:11.0; IEMobile/11.0; Microsoft; Virtual) like iPhone OS 7_0_3 Mac OS X AppleWebKit/537 (KHTML, like Gecko) Mobile Safari/537

It contains the string "Android" so it matches the first "Android" rule. Here's some explanation from Microsoft: http://blogs.msdn.com/b/ie/archive/2014/07/31/the-mobile-web-should-just-work-for-everyone.aspx

Even if it didn't contain "Android", it would match the "Windows Phone [0-9.]+" part of WindowsMobileOS. This isn't ideal - I assume we should be returning WindowsPhoneOS.

I wasn't sure whether to post this here or in the Mobile_Detect library... The os() function doesn't exist in the PHP library, and perhaps this is the reason...

hgoebl added a commit that referenced this issue Nov 12, 2014
@hgoebl
Copy link
Owner

hgoebl commented Nov 12, 2014

Microsoft is doing really crazy things. This adds enormous complexity for device detection. At least for this special case I "fixed" mobile-detect.js, but I won't support crazy vendors any longer. What if another vendor fakes his mobile device to be Android, iPhone, ...? Then the owners of these phones are victims to be seen as Android on one website and iPhone on another. I'd sell such a device or just throw it away.

@serbanghita
Copy link
Collaborator

@davelondon Nice catch. This is one of the cases we took into account when building the 3.0.0 version of Mobile_Detect. I am totally aware of that and thank you for this feedback!

I am trying to rush things up with the new detection engine which prioritize the browser/os/model detection.

@hgoebl I will notify you by email when devel-3 branch is ready to be tested, right now we have some commits pending.

@hgoebl
Copy link
Owner

hgoebl commented Nov 13, 2014

@serbanghita you're the man! Thanks for all your work!

@dave
Copy link
Author

dave commented Nov 17, 2014

Hey guys,

Have you thought of tweaking this library so it's a more exact port of the logic of the PHP version? We use this and the PHP version in tandem, and issues with the slightly different logic could make for some nasty bugs in edge cases...

It seems that by design, more than one OS can be applicable to each device (WindowsPhoneOS seems to be a subset of WindowsMobileOS)... This means we have to rely on the order of the oss rules object. We shouldn't really rely on the order of a javascript object

I tried to re-write my code to use the is() function instead of os(), but on closer inspection, the is() function is actually using os() under the hood.

How would you like me to do a patch that re-wires the is() function to match the logic of the php is* functions - which uses matchUAAgainstKey

@hgoebl
Copy link
Owner

hgoebl commented Nov 18, 2014

Hi @davelondon,
I'm sorry but quite at this moment I'm very busy with my day-time work. Do you have a solution which wouldn't break, but only improve the current one? If yes, I'd be glad to merge it. After begin of December I'll have time again.

BTW I don't understand the subtle differences between WindowsPhoneOS and WindowsMobileOS, but I admit that I ignored Windows on mobile devices too much until now.

@hgoebl hgoebl reopened this Nov 18, 2014
@serbanghita
Copy link
Collaborator

@davelondon omg nice catch! This means that the order of keys in the JSON object is meaningless ... this is crucial for the logic of the script.

I will not make this mistake for the 3.0.0, I need to talk to Nick about this and get back. 🏃

@hgoebl
Copy link
Owner

hgoebl commented Mar 22, 2015

already fixed in 0.4.3

@hgoebl hgoebl closed this as completed Mar 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants