Posting the D8 port from tabbabi's github here. https://github.com/tabbabi/detect_mobile

It needs to be redone to work with composer for the third party library file.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

HongPong created an issue. See original summary.

HongPong’s picture

Title: Dupal 8 port from detect_mobile » Drupal 8 port from detect_mobile
FileSize
77.65 KB

as noted here https://github.com/tabbabi/detect_mobile/issues/3 it does not work with regular caching.

I set info.yml to 'layout' module package.

Most importantly this should be changed to gather the MobileDetect.php through composer. Both the source module and the library are MIT license. I added credit to tabbabi in the Readme.md which is renamed from readme.txt. I tried to catch all the machine names and reverse them to match this module. So this covers most of the necessary work.

I would assume that this would need to bust the drupal page cache for every client, which I don't see any hint of, but the author noted it does not work with regular Drupal cache.

HongPong’s picture

Issue summary: View changes
Prashant.c’s picture

Resubmitting the patch :

1. Found a thepatch.patch file in the patch, removed the file.
2. Changed hook_help() name, from detect_mobile to mobile_detect.

Prashant.c’s picture

Status: Needs work » Needs review
mpdonadio’s picture

Status: Needs review » Needs work

Nice start!

  1. +++ b/mobile_detect.info.yml
    @@ -1,5 +1,5 @@
    -description: 'Lightweight mobile detection based on Mobile_Detect.php'
    +description: A wrapper around the "Mobile_Detect.php" library - Detects mobile devices using HTTP headers, proving helper functions to developers for templates.
     core: 8.x
    -package: 'Mobile Detect'
    +package: Layout
    

    Not sure if I like these changes.

  2. +++ b/mobile_detect.module
    @@ -1 +1,36 @@
    +function mobile_detect_help($route_name, RouteMatchInterface $route_match)
    +{
    +  switch ($route_name) {
    

    This applies in a bunch of places, but we need to make sure we apply Drupal coding standards everywhere.

  3. +++ b/mobile_detect.module
    @@ -1 +1,36 @@
    +    break;
    +  }
    

    Don't need the break;

  4. +++ b/mobile_detect.module
    @@ -1 +1,36 @@
    +function mobile_detect_theme()
    +{
    +  $theme = [];
    +
    +  return $theme;
    +}
    

    Not really sure why this is needed?

  5. +++ b/src/Detect/MobileDetect.php
    @@ -0,0 +1,10 @@
    +require_once 'Mobile_Detect.php';
    
    +++ b/src/Detect/Mobile_Detect.php
    @@ -0,0 +1,1264 @@
    +<?php
    +/**
    + * Mobile Detect Library
    + * =====================
    + *
    

    The Mobile_Detect.php library should be in composer.json and not the patch, and will then be autoloaded.

  6. +++ b/src/Detect/MobileDetect.php
    @@ -0,0 +1,10 @@
    +class MobileDetect extends \Mobile_Detect {}
    

    Like the idea of extending the class here. Not quite sure if I like the namespace. Need to look at a few other services for inspiration.

  7. +++ b/src/Twig/MobileDetectExtension.php
    @@ -0,0 +1,98 @@
    +class MobileDetectExtension extends \Twig_Extension
    

    Nice. We'll need a CacheContext to go along with this, though. Not really sure how Twig extensions and CacheContexts go together, though.

  8. +++ b/src/Twig/MobileDetectExtension.php
    @@ -0,0 +1,98 @@
    +        $magicMethodName = 'is' . strtolower((string) $deviceName);
    +
    +        return $this->mobileDetector->$magicMethodName();
    +    }
    

    Should probably use `call_user_func_array` here, and the method needs some error checking. May want to thrown an exception if the the $deviceName doesn't exist.

Prashant.c’s picture

@mpdonadio

Very nice feedback given.

I will try to work on as much as possible on these inputs.

mpdonadio’s picture

I think we may also want to check with tabbabi about using this. One, that repo is MIT, so this would be relicensed it as GPLv2. Two, I want to be sure all credit is given appropriately.

Prashant.c’s picture

1. One, that repo is MIT, so this would be relicensed it as GPLv2. (@mpdonadio You can take care of this one )
2. Two, I want to be sure all credit is given appropriately. (All contributors should take care of this one and give credits appropriately )

HongPong’s picture

Hi mpdonadio I thought it made sense to move it to the layout package because mobile detection would primarily effect layout markup and that was where the D8 source module info.yml put it. I don't feel strongly about it but suggest it's worth considering.

Also I emailed tabbabi based on his github email address and gave him this issue URL so hopefully he will weigh in with us about licensing. Best regards.

HongPong’s picture

HI everyone, tabbabi confirmed to me in an email the following:
"Thank you for your feedback on my D8 module, I'm glad you are working on it.
Absolutely, you can change the license to GPLv2.
I looked at your feedback, I totally agree, especially for the use of the composer to install third library, I already made the change locally and I commit to my github if you like."

So I asked if they can pop it on the github, then that part will be done.

mpdonadio’s picture

Inquiring about official procedure in #2925407: Procedure for relicensing a MIT project from GitHub. Also have a message in Slack about it. If we don't hear back in a reasonable amount of time, I'll contact a LWG member directly. May take a few bit b/c the holiday in the US.

mpdonadio’s picture

Category: Task » Plan
Status: Needs work » Active

We are clear on the licensing now, https://github.com/tabbabi/detect_mobile/commit/81b9cca39369a892a61f8c98...

If someone wants to post a patch w/o the Mobile_Detect.php code and preferable a pass though phpcs w/ autofixes, I am going to split it up into sub-issues for a better commit history.

marwensf’s picture

I did the PR on github to use the composer for the third party library .
TODO: Unit tests 100% code coverage

mpdonadio’s picture

#14, we don't need to do that to move forward. It's already in the composer.json for the mobile_detect module. But yes, we will need tests.

We just need a starting patch

- w/o Mobile_Detect.php
- w/ as many coding standards fixes as possible (I'd be happy with phpcs auto-fix)

Once we have that, I will start some sub issues, parse out and commit things, and we can kickstart this module again.

nonom’s picture

Status: Active » Reviewed & tested by the community
nonom’s picture

Status: Reviewed & tested by the community » Fixed

I should mark this as fixed. It was tested during long time in a production environment.

nonom’s picture

Status: Fixed » Closed (fixed)