[3.x] Fix undefined array key 1 for user agent without version in "detectEngine" method of web client #132
Conversation
|
|
||
| if ($version[0] >= 28) { | ||
| if ($version === false || $version[0] >= 28) { |
There was a problem hiding this comment.
Maybe it should be the same as AppleWebKit? I assume self::WEBKIT to be the fallback value.
$version !== false &&
There was a problem hiding this comment.
I would assume an actual version if no version is given. "Chrome Privacy Preserving Prefetch Proxy" is a real user agent used for the https://developer.chrome.com/blog/private-prefetch-proxy feature of Google Chrome.
There was a problem hiding this comment.
I've added a unit test, and it fails with a reason. The detectBrowser method would need to be adjusted to not detect a version="Privacy " when the user agent is "Chrome Privacy Preserving Prefetch Proxy"
There was a problem hiding this comment.
Changed to draft status until I have checked further.
There was a problem hiding this comment.
I thin it is OK to use if ($version === false || $version[0] >= 28) { here as the https://developer.chrome.com/blog/private-prefetch-proxy has been added to Chrome 103 for Android, and for other cases without version we can also assume Chrome version >= 28.
There was a problem hiding this comment.
Looks like this should work. (I'm currently not able to test this, sorry.)
Add unit test data for user agent "Chrome Privacy Preserving Prefetch Proxy" for web client.
Don't detect version='Privacy' in detectBrowser method of web client.
|
|
||
| if ($version[0] === 537.36) { | ||
| if ($version !== false && $version[0] === 537.36) { | ||
| // AppleWebKit/537.36 is Blink engine specific, exception is Blink emulated IEMobile, Trident or Edge | ||
| $this->engine = self::BLINK; |
There was a problem hiding this comment.
Not related to your PR, but this doesn't seem right. The assignment on line 412 is replaced on line 417.
There was a problem hiding this comment.
Yes ... weird. Would have to check with Git Blame if someone has removed a "return" statement or how else that code was changed so it is that weird.
There was a problem hiding this comment.
It was added with this PR in 2016: #62 . And that created that nonsense.
There was a problem hiding this comment.
Move line 417 above the if statement (line 406) should fix the issue.
There was a problem hiding this comment.
or put it into an else clause:
} elseif (\stripos($userAgent, 'AppleWebKit') !== false || \stripos($userAgent, 'blackberry') !== false) {
if (\stripos($userAgent, 'AppleWebKit') !== false) {
...
}
else
{
// Evidently blackberry uses WebKit and doesn't necessarily report it. Bad RIM.
$this->engine = self::WEBKIT;
}
Pull Request for Issue #112
Summary of Changes
This pull request (PR) adds a check if a version part was found in the
$userAgentparameter at 2 places in the "detectEngine" method of web client in order not to run into a PHP error "Undefined array key 1" when a known user agent is used without a version part.See the linked issue and another CMS issue joomla/joomla-cms#44469 .
In addition, it modifies the "detectBrowser" so that for the user agent "Chrome Privacy Preserving Prefetch Proxy" no version "Privacy" is detected.
The user agent "Chrome Privacy Preserving Prefetch Proxy" is used by the https://developer.chrome.com/blog/private-prefetch-proxy and does not provide a version.
Finally, this PR adds a unit test case for that user agent (and will possibly add more test cases for user agents without version).
To be done
Possibly further test cases for user agents without version.
Testing Instructions
Use a user agent equal to e.g. "Chrome Privacy Preserving Prefetch Proxy" or just "Chrome", or e.g. just "AppleWebKit" without any version part appended with "/".
Result without this PR: PHP warning
Undefined array key 1and then PHP Deprecatedexplode(): Passing null to parameter #2 ($string) of type string is deprecated.Result with this PR: No such warning and deprecated message.
Review the unit test case added by this PR and check that it passes in Drone for this PR.
Documentation Changes Required
None as far as I know.