Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix detecting device type by form factor of client hints #7843

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

Nommyde
Copy link
Contributor

@Nommyde Nommyde commented Sep 21, 2024

Description:

In the method \DeviceDetector\Parser\Device\AbstractDeviceParser::parseClientHints,
condition \array_key_exists($formFactor, $formFactors) is always false because $formFactor is a string and $formFactors keys are integers ($formFactors is a list of strings). If we fix just this condition, then $deviceType gets the string value of self::getDeviceName, but it should be an int (we don't need the getDeviceName call)
Then, if we fix that, we see that 'deviceType' field is never used in the \DeviceDetector\Parser\Device\AbstractDeviceParser::parse method, so we need to fill $this->deviceType with it to get advantage of client hints when nothing else helps to detect device type.

I am not sure if the fixture I created fits your conventions, but it detects the described problems and does its job.

Comment on lines 2302 to 2308
foreach ($formFactors as $formFactor) {
if (isset(self::$clientHintFormFactorsMapping[$formFactor])) {
$deviceType = self::$clientHintFormFactorsMapping[$formFactor];

break;
}
break;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

            foreach (self::$clientHintFormFactorsMapping as $formFactor => $deviceTypeId) {
                if (\in_array($formFactor, $formFactors)) {
                    $deviceType = $deviceTypeId;

                    break;
                }
            }

Copy link
Contributor Author

@Nommyde Nommyde Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that we need to consider priority. But if the header contains "EInk", "Watch" or "Desktop", "EInk", we'll recognize it as a tablet. Should we maybe lower the priority of eink?

Copy link
Collaborator

@sanchezzzhak sanchezzzhak Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can set, after desktop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I have updated the priorities and modified the fixture so that it will catch changing that algorithm to my previous version or the sorting of $clientHintFormFactorsMapping

@sanchezzzhak sanchezzzhak merged commit 4df1139 into matomo-org:master Sep 23, 2024
15 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants