Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix - package broken due to dependecy code change #208

Closed
wants to merge 1 commit into from
Closed

fix - package broken due to dependecy code change #208

wants to merge 1 commit into from

Conversation

driade
Copy link

@driade driade commented Nov 2, 2023

Hi.

Surely due to this change

serbanghita/Mobile-Detect@ba9281b

the package is broken as we add to "getOperatingSystems" a list of not really mobile ones.

Hope this helps.

cc #207

@driade
Copy link
Author

driade commented Nov 2, 2023

cc @ash-jc-allen/short-url#222

@driade
Copy link
Author

driade commented Nov 2, 2023

A temporary solution for those affected would be sticking mobiledetect/mobiledetectlib to version 2.8.41

composer require mobiledetect/mobiledetectlib 2.8.41

@driade
Copy link
Author

driade commented Nov 2, 2023

Long explanation

This code prints "1" with mobiledetect 2.8.43, and "" with 2.8.41

use Jenssegers\Agent\Agent;

$agent = new Agent();
$agent->setUserAgent('Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/537.13+ (KHTML, like Gecko) Version/5.1.7 Safari/534.57.2');
echo $agent->isPhone();

This is due to a change in mobiledetect, that instead of using an internal list of mobile operating systems, uses the function getOperatingSystems().

public static function getOperatingSystems()
{
    return self::$operatingSystems;
}

This function is overriden by jenssegers/agent, which adds a few more operating systems

public static function getOperatingSystems()
{
        return static::mergeRules(
            static::$operatingSystems,
            static::$additionalOperatingSystems
        );
}

These additionalOperatingSystems on jenssen are not mobile ones.

 protected static $additionalOperatingSystems = [
        'Windows' => 'Windows',
        'Windows NT' => 'Windows NT',
        'OS X' => 'Mac OS X',
        'Debian' => 'Debian',
        'Ubuntu' => 'Ubuntu',
        'Macintosh' => 'PPC',
        'OpenBSD' => 'OpenBSD',
        'Linux' => 'Linux',
        'ChromeOS' => 'CrOS',
    ];

@derekmd
Copy link

derekmd commented Nov 2, 2023

The same issue also exists for the getBrowsers() method override. e.g., any user agent string containing "Firefox" or "Chrome" returns Agent::isMobile() === true and Agent::isDesktop() === false, despite not having a mobile device indicator in the string.

The Agent class getter method overrides would need to be renamed and/or removed.

For this package:

  1. 1.x likely just needs to pin composer require mobiledetect/mobiledetectlib:2.8.41 and archive that branch.
  2. 2.x could remove the method overrides and upgrade mobiledetect/mobiledetectlib that is currently on v4.

This package hasn't seen maintenance since 2021 so a community initiative is likely needed.

@serbanghita
Copy link

today: I'm reverting the change in Mobile Detect, will keep you posted.

@driade
Copy link
Author

driade commented Nov 7, 2023

@serbanghita Thank you very much, this is really appreciated.

@serbanghita
Copy link

Should be fixed without intervention to jenssegers/agent

#207 (comment)

@driade
Copy link
Author

driade commented Nov 8, 2023

Thanks again @serbanghita , I'm closing this pull request

@driade driade closed this Nov 8, 2023
@driade driade deleted the feature/fix_mobile_detection branch November 8, 2023 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants