Bug: How can I know which browser name is the request from?

[warmbook]

PHP Version

8.1

CodeIgniter4 Version

4.5.0

CodeIgniter4 Installation Method

Manual (zip or tar.gz)

Which operating systems have you tested for this bug?

Linux

Which server did you use?

apache

Database

No response

What happened?

When I call the method CodeIgniter\HTTP\UserAgent->isBrowser with 'Spartan', if the client browser agent string matched the keyword 'Edge', the method will return false.

Steps to Reproduce

Call CodeIgniter\HTTP\UserAgent->isBrowser with 'Spartan', and request the server by a browser which's agent string includes 'Edge'.

Expected Output

method should return true

Anything else?

By viewing the source code,I found this method only check whether the param $key is existed in user agent string, but not compare it with the property 'browser'. It's not compliant with description in the manual, which say the param $key is 'Optional browser name'. Maybe you should change the statement "if($key === null)" to "if ($key === null||$this->browser === $key)"?
The other two method 'isRobot' and 'isMobile' also have the same problem.

[kenjis]

Call isBrowser () without $key.
There is no key Spartan in $browsers:

CodeIgniter4/app/Config/UserAgents.php

Line 81 in 482b8bb

public array $browsers = [

Note
The string “Safari” in this example is an array key in the list of browser definitions. You can find this list in app/Config/UserAgents.php if you want to add new browsers or change the strings.
https://codeigniter4.github.io/CodeIgniter4/libraries/user_agent.html#CodeIgniter\HTTP\UserAgent::isBrowser

[kenjis]

How can I know which browser name is the request from?

In a controller:

$agent = $this->request->getUserAgent();
d($agent->getBrowser());

[kenjis]

Ah, the following $agent->isBrowser(...) result is not correct?

        $agent = $this->request->getUserAgent();
        d($agent->getBrowser());
        echo($agent->getAgentString());
        dd($agent->isBrowser('Edge'));

$agent->getBrowser() string (4) "Edge"
⧉Called from .../app/Controllers/Home.php:11 [d()]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/125.0.0.0 Safari/537.36 Edg/125.0.0.0

$agent->isBrowser(...) boolean false
⧉Called from .../app/Controllers/Home.php:13 [dd()]

[kenjis]

@codeigniter4/core-team
According to the documentation, the parameter for isBrowser() is "Optional browser name".
See https://codeigniter4.github.io/CodeIgniter4/libraries/user_agent.html#CodeIgniter\HTTP\UserAgent::isBrowser

But it is actually the key of Config\UserAgents::$browsers, and the key of $browsers is (a part of) the regex for the browser. So it is NOT a browser name.

CodeIgniter4/system/HTTP/UserAgent.php

Lines 310 to 323 in 482b8bb

	protected function setBrowser(): bool
	{
	if (is_array($this->config->browsers) && $this->config->browsers) {
	foreach ($this->config->browsers as $key => $val) {
	if (preg_match('|' . $key . '.*?([0-9\.]+)|i', $this->agent, $match)) {
	$this->isBrowser = true;
	$this->version = $match[1];
	$this->browser = $val;
	$this->setMobile();
	return true;
	}
	}
	}

After all, the current implementation (line 133) seems to be incorrect.
What do you think?

CodeIgniter4/system/HTTP/UserAgent.php

Lines 121 to 134 in 482b8bb

	public function isBrowser(?string $key = null): bool
	{
	if (! $this->isBrowser) {
	return false;
	}
	// No need to be specific, it's a browser
	if ($key === null) {
	return true;
	}
	// Check for a specific browser
	return isset($this->config->browsers[$key]) && $this->browser === $this->config->browsers[$key];
	}

[michalsn]

The user guide states:

The string “Safari” in this example is an array key in the list of browser definitions. You can find this list in app/Config/UserAgents.php if you want to add new browsers or change the strings.

https://codeigniter4.github.io/CodeIgniter4/libraries/user_agent.html#CodeIgniter%5CHTTP%5CUserAgent::isBrowser

So, I would say we're good.

[kenjis]

Yes, the Note says it is an array key.
But if the current implementation is good, we must write code $agent->isBrowser('Edg') instead of $agent->isBrowser('Edge').
It is difficult to use/understand.

[warmbook]

Yes, the Note says it is an array key. But if the current implementation is good, we must write code $agent->isBrowser('Edg') instead of $agent->isBrowser('Edge'). It is difficult to use/understand.

Yes, It's indeed difficult to use/understand. Especially when some browsers can't be specifyed by pure letter string ,such as QQ, we must write code $agent->isBrowser('QQ/'), that's not elegant.

[michalsn]

Perhaps, but the behavior is the same since probably v2. There is no bug, the method works correctly.

Feel free to suggest a better option (preferably with PR), but please make it an alternative. The current behavior should remain unchanged - too many apps may rely on it.

[warmbook]

In my humble opinion, change the statement "if($key === null)" to "if ($key === null||$this->browser === $key)" may not affect apps rely on it?

[michalsn]

I kindly disagree. If we start matching the result based on both: the array key and the value, there is no chance to determine which value was matched.

"Edge" is a great example of this:

'Edge'   => 'Spartan',
'Edg'    => 'Edge',

If we change the code according to your suggestion, it will give unexpected results.

$agent->isBrowser('Edge');

If the above code returns true, we don't know if it's because the browser is indeed "Edge" or "Spartan".

To check a browser by its name, I suggest you simply use:

$agent->getBrowser() === 'Edge'

[kenjis]

The format is the same as the Chromium user agent with the addition of a new Edg token at the end. Microsoft chose the Edg token to avoid compatibility issues caused by Edge string, which was previously used for the legacy Microsoft Edge browser based on EdgeHTML. The Edg token is also consistent with existing tokens used for iOS and Android.
https://learn.microsoft.com/en-us/microsoft-edge/web-platform/user-agent-guidance

[kenjis]

@michalsn Indeed, if we think the parameter for isBrowser() is an array key (actually it is),
there is no inconsistency.

But if we want to check if it is "Spartan", we must write:

$agent->isBrowser('Edge')

and check if it is "Edge", we must write:

$agent->isBrowser('Edg')

This seems too tricky.

It seems the following is better and simpler:

--- a/system/HTTP/UserAgent.php
+++ b/system/HTTP/UserAgent.php
@@ -130,7 +130,7 @@ class UserAgent implements Stringable
         }
 
         // Check for a specific browser
-        return isset($this->config->browsers[$key]) && $this->browser === $this->config->browsers[$key];
+        return $this->browser === $key;
     }
 
     /**

[michalsn]

@kenjis It might be less intuitive, but this is how it was designed to work and it's well-documented.

         // Check for a specific browser
-        return isset($this->config->browsers[$key]) && $this->browser === $this->config->browsers[$key];
+        return $this->browser === $key;

This change would completely break the code logic for some apps. Checking $agent->isBrowser('Edg') would simply fail.

[warmbook]

@michalsn Thanks! I have got it.