serbanghita / Mobile-Detect

Mobile_Detect is a lightweight PHP class for detecting mobile devices (including tablets). It uses the User-Agent string combined with specific HTTP headers to detect the mobile environment.
http://mobiledetect.net
MIT License
10.55k stars 2.68k forks source link

The mobileGrade() function never reaches the default return statement #765

Closed luisamat closed 2 years ago

luisamat commented 6 years ago

https://github.com/serbanghita/Mobile-Detect/blob/9713866c051565de918b19c4ad03535af1e00e87/Mobile_Detect.php#L1454 and https://github.com/serbanghita/Mobile-Detect/blob/9713866c051565de918b19c4ad03535af1e00e87/Mobile_Detect.php#L1456

The mobileGrade() function never reaches the default return statement at the end of the function because of the BlackBerry and Windows Mobile version checks in the last if block. As a result, the last if block is either not needed or is simply not useful.

CAUSE: As currently implemented, invokers of $this->version(...) cannot distinguish between either that function returning false (eg: when $this->is('BlackBerry') == false) or returning a version greater than x (eg: > 5.0). Both cases yield a logical false.

OBFUSCATION: This problem is somewhat hidden and particularly difficult to notice because:

  1. In other parts of the code, all conditional statements containing an invocation of $this->version(...) always seem to be coupled with && terms. These && terms, effectively, eliminate the side-effect which causes this issue.
  2. The return value of the last if block and the value returned by default (at the end of the function) are the same. This means that mobile-detect consumers will not even notice the issue because they will always get MOBILE_GRADE_C regardless of the outcome of the last if statement.

THE CASE FOR A FIX: Regardless of the end results being identical irrespective of how the last if block or the version(...) function are implemented, this is most certainly a bug that may surface later if you decide to, for example, add a new grade for the last if block or the default return case (as we have for our use case).

POSSIBLE FIXES: Either of the following approaches might address the issue:

hgoebl commented 6 years ago

Not answering the technical problem. But: mobileGrade nowadays doesn't play any role. Effectively, when we provide web pages, they will be seen by Android 75%, iPhone/iPad 24.99%. Who cares about 0.01%? It's all Grade A. I suppose nobody ever uses this function. Nobody uses grade B / C / none devices.

luisamat commented 6 years ago

Agreed in principle for web pages, but we are using mobile detect (along with some other heuristics) to help decide the appropriate video format for various classes (grades) of devices.

As for percentages of smartphones versus features phones...

According to Statista: While "[t]he number of mobile phone users in the world is expected to pass the five billion mark by 2019", the number of smartphone users is expected to reach 2.71 billion users. That leaves a not insignificant number of non-smartphones in the world.

According to Quartz Africa, "the market share of feature phones [in Africa] rose to 61% in 2017 from 55.4% in 2016, while market share for smartphones fell to 39% from 44.6%, according to data provided by IDC."

hgoebl commented 6 years ago

Interesting facts - I didn't expect such numbers. IMHO the question is how many users of features phones visit web resources or watch videos and how often do they use the internet compared to smartphone users.

I'm just a bit curious because in the JavaScript library mobile-detect.js I use the exact same code (it's just a port of PHP to JavaScript).

luisamat commented 6 years ago

Yeah - pretty shocking numbers indeed. To answer your question, our company, VMS Communications, and platform, LinkShot (currently in private beta), delivers links to content, landing pages, etc, directly to the end user via SMS. That's how we get to those devices. Our approach is to use the same link to deliver the same content as -- in the case of video -- either HLS, MP4 or 3GPP depending on a realtime determination of the capabilities of the client.

We've seen the JS library, but we are using the PHP version for our current architecture since we have to determine the device capabilities before serving the final content.