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.54k stars 2.68k forks source link

Fix deprecation error in PHP 8.1 #875

Closed jdanino closed 2 years ago

jdanino commented 2 years ago

Fix errors in php8.1 : preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated in

francislavoie commented 2 years ago

This isn't correct, when $userAgent is empty, it may use $this->userAgent instead.

A more correct fix (note the $subject &&):

--- a/Mobile_Detect.php 2021-11-26 20:24:58.942859134 +0000
+++ b/Mobile_Detect.php 2021-11-26 20:25:08.058923323 +0000
@@ -1457,7 +1457,8 @@
      */
     public function match($regex, $userAgent = null)
     {
-        $match = (bool) preg_match(sprintf('#%s#is', $regex), (false === empty($userAgent) ? $userAgent : $this->userAgent), $matches);
+        $subject = (false === empty($userAgent) ? $userAgent : $this->userAgent);
+        $match = $subject && (bool) preg_match(sprintf('#%s#is', $regex), $subject, $matches);
         // If positive match is found, store the results for debug.
         if ($match) {
             $this->matchingRegex = $regex;
DrLightman commented 2 years ago
false === empty(...) ? ...

Is this necessary? empty already returns a bool itself. Just use !empty or empty itself for the condition, is way more readable?

francislavoie commented 2 years ago

Readability is subjective. To some, false === is more readable than ! because it's easier to notice the negation (I'm of the space before ! camp myself). But either way, I'd rather retain the original "style" than change it. I don't want to make any assumptions about what the maintainer will accept, so I prefer to keep it the same.

dereuromark commented 2 years ago

Either way, we need to remove the deprecation warnings thrown, as they make currently a lot of noise in test suites.

darapa1 commented 2 years ago

There is a second pull request started with issue #876 with an easy fix (just return false in case of empty UA string). This solution works fine and all warnings and deprecated message are gone (with the addition of my comment in the pull request.)

julienbornstein commented 2 years ago

thanks @darapa1. For an unknown reason my PR was not target to the main project but on my fork 😑. I've just open the PR to be merged in this project. Check PR #879