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

isMobile doesn't cache matches #935

Closed npelov closed 1 year ago

npelov commented 1 year ago

Is there any reason isMobile() method doesn't cache matches in matchDetectionRulesAgainstUA()?

Issue description

If you call isMobile() and then isIphone it'll run match again while it could be cached. Also if you run isIPhone() the result will be cached, but then if you run isMoible() the cache won't be used.

Suggestions

As a side note - I'm not sure why match() method is public. it is not useful since by itself it doesn't detect anything.

Warning! DO NOT take this code as is - it's quickly written and probably won't work correctly. It's only meant to give you ideas.

You could implement internal match (protected), Ether make match that method or make a new one like runMatchWithCache(). This method should look something like this:

    protected function runMatchWithCache(string $keyName, string $regex, string $userAgent = null): bool
    {
      if (!\is_string($userAgent) && !\is_string($this->userAgent)) {
        return false;
      }

      $keyName = strtolower($keyName);

      if(!isset($this->cache[$keyName])) {
        $match = (bool) preg_match(
          sprintf('#%s#is', $regex),
          empty($userAgent) ? $userAgent : $this->userAgent,
          $matches
        );
        // If positive match is found, store the results for debug.
        if ($match) {
          $this->matchingRegex = $regex;
          $this->matchesArray = $matches;
        }

        $this->cache[$keyName] = $match; // store in cache
      }
      return $this->cache[$keyName];

    }

Then use this function inside matchUAAgainstKey like this:

    protected function matchUAAgainstKey(string $key): bool
    {
        $_rules = array_change_key_case($this->getRules()); // move this to getRules();

      $key = strtolower($key);

      if(!empty($_rules[$key])) // you don't need false === here because empty() always returns boolean
        return $this->runMatchWithCache($key, $_rules[$key], $this->userAgent);

      return false;
    }
serbanghita commented 1 year ago

Is there any reason isMobile() method doesn't cache matches in matchDetectionRulesAgainstUA()?

This is a mistake on my part, I should have cached this, but probably at that point I realized that my cache key should be based on userAgent string value. In matchUAAgainstKey I used the name of the method which is probably not that great to begin with. I probably avoided to create a cache key based on User Agent string + HTTP headers array

If you call isMobile() and then isIphone it'll run match again while it could be cached. Also if you run isIPhone() the result will be cached, but then if you run isMoible() the cache won't be used.

As a side note - I'm not sure why match() method is public. it is not useful since by itself it doesn't detect anything.

Should be protected, probably I was thinking to allow people to use custom regexes. Might be a good idea to make it protected and then whoever extends MobileDetect can work with it. This is clearly a bug. setUserAgent resets the $cache. I will fix this and cache based on User Agent string + HTTP headers array

serbanghita commented 1 year ago

I have fixed this in https://github.com/serbanghita/Mobile-Detect/releases/tag/4.8.01 This is a major refactor, I added support for PSR-16 cache

@npelov Nikolay, thank you for the detailed write-up, it helped me get motivated to release this new version with fixes.