search

jenssegers / agent

👮 A PHP desktop/mobile user agent parser with support for Laravel, based on Mobiledetect
https://jenssegers.com
MIT License
4.53k stars 473 forks source link

Browser version detection fails for 'Opera' and 'IE' if user agent string doesnt match #138

Closed mpetty closed 5 years ago

mpetty commented 5 years ago

The version function updates in 0.6.2 caused errors to be thrown when the user agent string passed in doesn't match the browser being tested if you test the version of 'Opera' or 'IE'. I didnt notice it on any other browsers.

how to reproduce: $agent = new Agent(); $agent->setUserAgent('Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322)'); // IE user agent $agent->version('Opera');

error info: https://github.com/jenssegers/agent/blob/41cd1fd6200a9b0d70db75133ee23064c9965980/src/Agent.php#L326

'Array to string Conversion' jenssegers/agent/src/Agent.php(326): sprintf('#%s#is', Array)

mpetty commented 5 years ago

Its only failing when the browser doesnt match the user agent because its getting all the way through the '$propertyPattern' test list. The mergeRules method is not fully flattening the 2 lists from this library and the 'mobile_detect' library. So the last item is an array when it should be a string.

peter279k commented 5 years ago

@mpetty, thank you for your reply.

It looks like we have to handle/throw the exception when the browser doesn't match any user agent.

aboudeh87 commented 5 years ago

I have the same issue

mpetty commented 5 years ago

@peter279k While handling this error using try/catch around the 'version' method is fine if its documented, it's not optimal. Im using the agent->browser() method to return the browser and pass it into the version, but for some reason it will occasionally not match and throw this error.

This seems like an unintended bug while merging the rules from 'agent' and 'mobile detect'. Adding this line to the mergeRules method right after line 351 seems to fix it. I can make a pull request if you prefer.

if (is_array($value)) $value = implode('|', $value);

peter279k commented 5 years ago

@mpetty, thank you for your reply.

If possible, I glad you can create the PR about fixing this issue :+1: .

peter279k commented 5 years ago

@mpetty, thank you for your reply.

I think we should only throw exception when the detected user agent version is not same as specific version.

mpetty commented 5 years ago

@peter279k i see. mergeRules is used all over the place so maybe its safer if the code goes directly into the 'version' method. I'll make another change in the PR.

I was using the detected browser from the user agent string when i saw this error so i'm not sure throwing a specific error would be easy.

$agent->version($agent->browser());