Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#4 | drupal-8-mobile-detect-8.x-2.x-dev-3.patch | 75.17 KB | Prashant.c |
#2 | tabbabi-2-2924457.patch | 77.65 KB | HongPong |
Comments
Comment #2
HongPong CreditAttribution: HongPong as a volunteer and at kor group commentedas 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.
Comment #3
HongPong CreditAttribution: HongPong as a volunteer and at kor group commentedComment #4
Prashant.cResubmitting 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.
Comment #5
Prashant.cComment #6
mpdonadioNice start!
Not sure if I like these changes.
This applies in a bunch of places, but we need to make sure we apply Drupal coding standards everywhere.
Don't need the break;
Not really sure why this is needed?
The Mobile_Detect.php library should be in composer.json and not the patch, and will then be autoloaded.
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.
Nice. We'll need a CacheContext to go along with this, though. Not really sure how Twig extensions and CacheContexts go together, though.
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.
Comment #7
Prashant.c@mpdonadio
Very nice feedback given.
I will try to work on as much as possible on these inputs.
Comment #8
mpdonadioI 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.
Comment #9
Prashant.c1. 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 )
Comment #10
HongPong CreditAttribution: HongPong as a volunteer and at kor group commentedHi 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.
Comment #11
HongPong CreditAttribution: HongPong as a volunteer and at kor group commentedHI 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.
Comment #12
mpdonadioInquiring 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.
Comment #13
mpdonadioWe 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.
Comment #14
marwensf CreditAttribution: marwensf as a volunteer commentedI did the PR on github to use the composer for the third party library .
TODO: Unit tests 100% code coverage
Comment #15
mpdonadio#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.
Comment #16
nonom CreditAttribution: nonom as a volunteer commentedComment #17
nonom CreditAttribution: nonom as a volunteer commentedI should mark this as fixed. It was tested during long time in a production environment.
Comment #18
nonom CreditAttribution: nonom as a volunteer commented