[bug] No user-agent has been set

trendoman commented 1 year ago

Following pops up in PHP error log:

PHP Fatal error: Uncaught Detection\Exception\MobileDetectException: No user-agent has been set. in .../Mobile-Detect-4.8.03/src/MobileDetect.php:1369

trendoman commented 1 year ago

Now that I have checked that line 1369 I can see that it's not a bug but a feature 🥇 but it's spamming the log too much, so please give instruction to have that off.

https://github.com/serbanghita/Mobile-Detect/blob/80607ae92b5a8c77bb99a44bdee49f1d34de5af4/src/MobileDetect.php#L1368-L1370

serbanghita commented 1 year ago

@trendoman in what environment are you using this? In CLI, with Apache?

I just tested this with MobileDetect 4.8.03

$detect = new MobileDetect();
// $detect->setUserAgent($_SERVER['HTTP_USER_AGENT']);
$isMobile = $detect->isMobile();
$isTablet =- $detect->isTablet();
$deviceType = ($isMobile ? ($isTablet ? 'tablet' : 'phone') : 'computer');
$scriptVersion = $detect->getVersion();

var_dump($deviceType);
var_dump($scriptVersion);

and it works by automatically looking at $_SERVER

You could also explicitly setUserAgent like in the example https://github.com/serbanghita/Mobile-Detect/blob/4.8.x/scripts/example.php

trendoman commented 1 year ago

I have some PHP script code running at this dev environment [link edited out] and so far very few people come there to upload and test other scripts. It makes me worry bc on production I expect many more visitors. The line which produces exceptions is very basic, pretty much boils down to this —

$MB = new \Detection\MobileDetect();
var_dump( $MB->isMobile() );

I had a v.3 previously without issues, but recently tried the fresh MD 4.8.03 and now stuck with spam in error log. Any advice appreciated. Should I validate a non-empty user-agent for each visitor before creating MD object? That's possible, however I'd rather rely on MD to have that sorted out without throwing fatal errors, if that's ok with you.

Edit: remove link to private server

serbanghita commented 1 year ago

@trendoman From the phpini it seems that $_SERVER exists:

$_SERVER['HTTP_USER_AGENT'] | Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/118.0.0.0 Safari/537.36

I believe the $_SERVER variable is being deleted by the CMS and MobileDetect cannot auto-detect the HTTP_USER_AGENT header. I see a dump on your website to __ROOT__ array/object with the key of k_device_useragent.

In my case:

k_device_useragent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/118.0

try to set: $detect->setUserAgent(...);

serbanghita commented 1 year ago

You should never auto-update scripts that change major versions. In this case from 3.x to 4.x. The reason is that the packages might contain breaking changes. See semver

serbanghita commented 1 year ago

@trendoman you can go back to using 3.x, I'm still updating that version as well.

trendoman commented 1 year ago

Thanks, this advice to get back to 3.x confirms my understanding of the issue.

Just a few last notes FYI (and others that will search for this issue):

no, the $_SERVER variable is not being deleted by the CMS, because the CMS is not used (yet) at the point where I test MB
yes, you see a dump on the website to __ROOT__ array/object with the key of k_device_useragent — I added that dump output in case you stumble upon it (if visited Index out of curiosity) — as I set that key for every visitor and make it available to later CMS vars. The original problem is about random empty user-agent strings (perhaps from internet bots) which spam error log with Fatal error and Uncaught exception. Hence I can not set the UA unless adding extra code. Your choice of throwing an exception for every random bot without UA is thus questioned and is labelled as bug.
there is no any autoupdate of scripts in my usecase, but I agree that this advice is universally good so thanks

I'm probably going to resolve this (to keep using 4.x) by adding an extra try-catch around MB to avoid constant error log errors (from random bots/people without UA). I prefer error log for important stuff (much more for fatal error).

Thanks for your amazing work and attending to issues, nevertheless.

serbanghita commented 12 months ago

The original problem is about random empty user-agent strings (perhaps from internet bots) which spam error log with Fatal error and Uncaught exception.

Okay, now I fully understand the issue. When I changed the code behavior's in 4.x, I haven't thought about empty userAgent string scenario. I converted empty userAgent to null, thus throwing later an Exception. I think this is bad for developer experience.

I have released 4.0.04 which hopefully fixes your problem https://github.com/serbanghita/Mobile-Detect/releases/tag/4.8.04

Cheers 🙇

trendoman commented 12 months ago

Cheers 💯

This makes redundant an unecessary complication with try-catch and is - by far - more friendly to developers.

Now I can get back and throw away the ugly —

$MB = new \Detection\MobileDetect();

try {
    $isMobile = $MB->isMobile();
    $isTablet = $MB->isTablet();
} catch ( Exception $e ) {
    //https://github.com/serbanghita/Mobile-Detect/issues/946
} finally {
    $isMobile = ( $isMobile == 1 ) ? 1 : 0;
    $isTablet = ( $isTablet == 1 ) ? 1 : 0;
}

Have a great weekend!

penafelipe commented 12 months ago

Unsubscribed

bopoda commented 9 months ago

Hi guys. I would like to rise a question: does it make sense to throw an exception if User-Agent is empty? We can wrap with try/catch in our code when calling methods of the library, but that's not convenient. I did not catch, how can we throw it away?

Regarding web: on the one hand, that's logical that user-agent should exist when request comes. On the other hand, if User-Agent does not exist (is empty) then the backend should still return OK response in most of the cases, I guess. That's why I consider throwing exception as redundant thing. Can we just initialize with empty string?

Regarding this part of code: https://github.com/serbanghita/Mobile-Detect/blob/4.8.04/src/MobileDetect.php#L1088-L1108

// Setting new HTTP headers automatically resets the User-Agent.
// Set current User-Agent from known User-Agent-like HTTP header(s).
$userAgent = "";
foreach ($this->getUaHttpHeaders() as $altHeader) {
    if (!empty($this->httpHeaders[$altHeader])) {
        $userAgent .= $this->httpHeaders[$altHeader] . " ";
    }
}

if (!empty($userAgent)) {
    $this->setUserAgent($userAgent);
}

Empty string is not set if we pass empty string User-Agent in header, for example: curl -v http://127.0.0.1:8000/ -H "User-Agent: " So when handling http-request, if I call methods of library, I will receive an MobileDetectException.

Does it make sense to change:

if (!empty($userAgent)) {
    $this->setUserAgent($userAgent);
}

to:

$this->setUserAgent($userAgent);

And then remove this condition from all the methods?

if (!$this->hasUserAgent()) {
            throw new MobileDetectException('No user-agent has been set.');
}

The aim: avoid exceptions if the request was sent with empty User-Agent. Please correct me If I'm wrong.

trendoman commented 9 months ago

Please correct me If I'm wrong.

Not wrong. I personally found that I do not need exceptions if no user agent exists. If it does not exist I make it empty myself. Here is how I decided to do this in my app —

$MB = new \Detection\MobileDetect();
if( false === $MB->hasUserAgent() ){ $MB->setUserAgent(''); }

// then goes app's logic unrelevant to the topic but possibly a helpful context

if( !defined('K_USERAGENT_MOBILE') ) define( 'K_USERAGENT_MOBILE', $MB->isMobile() );
if( !defined('K_USERAGENT_TABLET') ) define( 'K_USERAGENT_TABLET', $MB->isTablet() );
unset($MB);

if( K_USERAGENT_MOBILE || K_USERAGENT_TABLET ) $k_cache_dir = K_COUCH_DIR . 'cache/mobile/';

As long as there is logic in your app about how to treat an empty ua, you are fine. The problem was with non-existent ua in the first place, which has been fixed by the @serbanghita

serbanghita commented 9 months ago

@bopoda Clearly this seems to be an issue for PHP users that I didn't expect to be

My current logic is:

new MobileDetect() auto-initiates discovery of HTTP headers and implicitly the User-Agent header.
If $_SERVER['HTTP_USER_AGENT'] + friends is found, then set $this->userAgent to a string value.
At this step $md->isMobile() or any other detection method throws Exception if $this->userAgent is not a string.

Programmatically speaking this is okay. From a UX pov it appears to be a bad decision.

I thought that the user wants a mechanism to be informed that User-Agent HTTP header is missing. I guess I can get rid of the exception since it might caught developers off-guard.

I believe the solution here is that when userAgent is not set on auto-initiation phase, I will set an empty string

serbanghita commented 9 months ago

if( false === $MB->hasUserAgent() ){ $MB->setUserAgent(''); }

@trendoman while this is fine, my aim was for users to use try/catch in PHP, I don't want you to be bothered to write extra code.

I think the point here is that the library exists for a long time and it has been used in a certain way and throwing exceptions is not really something that developers need in this case

serbanghita commented 9 months ago

I'll issue a patch to fix this behavior for when User-Agent is not present

trendoman commented 9 months ago

I think the point here is that the library exists for a long time and it has been used in a certain way and throwing exceptions is not really something that developers need in this case

I get it.

Quite anecdotally, I recently was able to find a solution to irrelevant issue in an app thanks to UA being not set e.g. caught a cURL request with a missing UA to a 404 page. This was nice to not have the UA empty in that particular case, because I was able to find the problem faster.

bopoda commented 9 months ago

$this->mobileDetect->setUserAgent('');

yeah, this call before "auto-discovery" helps to avoid Exception. In my opinion, invalid/missing header is not so big problem (but it depends).

I thought that the user wants a mechanism to be informed that User-Agent HTTP header is missing. I guess I can get rid of the exception since it might caught developers off-guard. I believe the solution here is that when userAgent is not set on auto-initiation phase, I will set an empty string

Yeah, got it, makes sense to inform the user somehow.

The case I would like to mention: If I have service and if someone sends http-requests with empty User-Agent -- FE 500 will be triggered if I don't handle it. It could trigger monitoring, spoil logs, etc. So I need to avoid 500 as this is the problem not from the server side.

wrap with try/catch (also, possible)
initialize mobileDetector with an empty string (I would prefer this one). So In general I can continue using it with no changes as we have this solution. It was a bit unexpected to see new Exceptions after the library update (yeah, but it's mentioned in new version of library)

Thank you!

serbanghita commented 9 months ago

New patch https://github.com/serbanghita/Mobile-Detect/releases/tag/4.8.05

By default it will no longer throw an Exception, it just sets the userAgent to empty string, see tests

https://github.com/serbanghita/Mobile-Detect/blob/b7a8cdd70955ea6162269939914ba97fe36a154a/tests/MobileDetectGeneralTest.php#L39-L56

It will throw an exception only when starting MobileDetect with 'autoInitOfHttpHeaders' set to false

https://github.com/serbanghita/Mobile-Detect/blob/b7a8cdd70955ea6162269939914ba97fe36a154a/tests/MobileDetectGeneralTest.php#L27-L37

serbanghita / Mobile-Detect

[bug] No user-agent has been set #946