[meta] Mobile Detect - Drupal 8 Port [#2443329]

Comment	File	Size	Author
#23	mobile_detect-23-2443329.patch	82.06 KB	abh.ai
#20	tabbabi-20-2443329.patch	77.65 KB	hongpong
#19	tabbabi-2443329-19.patch	318 bytes	hongpong
#2	2443329-experimental-code-for-mobile_detect_d8.patch	9.08 KB	darol100

Comment #1

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 28 February 2015 at 16:03

More than likely yes, but I don't know in exactly what form, and why I don't have a branch for it yet. There are three complications.

1. I am not sure if the current architecture makes sense in Drupal 8. But, I haven't put much thought into it.

2. I am not sure where the Libraries API vs composer for contrib stands right now. I would prefer to go the composer route, but I don't know where that stands for contrib.

3. There is a open Drupal 8 issue regarding content negotiation. Conneg is needed to properly support caching when you take the User-Agent into account. The WSCCI team hasn't decided on a solution for this yet. This really need to be ironed out before a Drupal 8 port, which to do right would need to create a middleware. We talked about this last week, but no real decisions were made.

Log in or register to post comments

Comment #2

darol100 commented 5 March 2015 at 10:13

Status	File	Size
new	2443329-experimental-code-for-mobile_detect_d8.patch	9.08 KB

2. I am not sure where the Libraries API vs composer for contrib stands right now. I would prefer to go the composer route, but I don't know where that stands for contrib.

I think composer would be the best direction to go base on what I have read. Personally, I do not have a lot experiences with Composer.

I know is very early stages but I was playing with the Drupal Module Upgrader and this module. I have follow all the recommendation that the Drupal Module Upgrader suggested in order to make this module compatible with D8. I have done few testing and I'm not sure if the module works or not yet. I created a patch just in case someone wants to try it out. It's very experimental I'm not even sure if works or not but this is an start of the D8 version.

Log in or register to post comments

Comment #3

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 5 March 2015 at 14:16

Status:

Active

» Needs review

I'll branch and commit this, so there is at least a starting point.

Log in or register to post comments

Comment #4

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 5 March 2015 at 20:30

Status:	Needs review	» Postponed
Issue tags:		+Needs reroll

This doesn't apply anymore. How about we wait until #1889824: Drupal Cache + Mobile Detect Conflict is marked as fixed and then generate a new version from that?

Log in or register to post comments

Comment #5

darol100 commented 5 March 2015 at 20:40

Sounds good to me ..

Log in or register to post comments

Comment #6

darol100 commented 9 March 2015 at 16:03

Related issues:

+#2448567: Mobile Detect Demo

I'm going to wait to see if this sub-module #2448567: Mobile Detect Demo is committed or not before I started working on this.

Log in or register to post comments

Comment #7

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 14 November 2015 at 14:56

Category:	Feature request	» Plan
Status:	Postponed	» Active
Issue tags:	-Needs reroll

More to come. I have plan worked out in my head.

Log in or register to post comments

Comment #8

darol100 commented 15 November 2015 at 11:05

@mpdonadio,

Responding to #1 . 2

I think we should use composer, base on conversation with many many many people that seem to be the future of php libraries in Drupal. And it seem like Libraries API is D8 direction is going to be for JS libraries.

3. There is a open Drupal 8 issue regarding content negotiation. Conneg is needed to properly support caching when you take the User-Agent into account. The WSCCI team hasn't decided on a solution for this yet. This really need to be ironed out before a Drupal 8 port, which to do right would need to create a middleware. We talked about this last week, but no real decisions were made.

Any new updates on this ?

There are two ways we can do a port.

We can either use the Drupal Module Upgrader and try to do straight port, which is what I try to earlier on #2. This will make the development faster but we probably are not talking full advantage of what new tools in D8
We can try to re-architect the entire project and we probably will have to open a lot issue to similar[#2398395]. This method is a little bit more complex but is going to be better.

I personally like the idea of 2, which is re-architect the entire project to see how much advantage we can get from new tools form D8.

I think that the analyzer of the Drupal Module Upgrader is a good start we probably want to run it and start fixing those issues individually on separate issues.

I'm removing the #2448567: Mobile Detect Demo issue because is a lower priority right now.

Log in or register to post comments

Comment #9

darol100 commented 15 November 2015 at 11:19

Title:	D8 Support ?	» [meta] Mobile Detect - Drupal 8 Port
Issue summary:	View changes
Related issues:	-#2448567: Mobile Detect Demo	+#2615714: [mobile_detect] Mobile_detect

Changing the title into a meta issue. I have also change the description with a more generic meta issue description.

In addition, I have added the relate issue #2615714: [mobile_detect] Mobile_detect for the Drupal 8 Contrib Porting Tracker.

Log in or register to post comments

Comment #10

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 15 November 2015 at 20:45

Version:

7.x-1.x-dev

» 8.x-2.x-dev

Updating version number to reflect D8, and the fact that this may not be 100% backwards compatible with 7.x-1.x.

Log in or register to post comments

Comment #11

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 18 November 2015 at 00:14

OK, I think the initial steps involve three parts

1. Wire up the module to use Compose Manager to load the Mobile_Detect library.
2. Create a service to abstract the Mobile_Detect library, probably as a thin class w/ an interface. However, I am not sure how/if you can avoid using a singleton here, which would make unit testing the service harder.
3. Create new cache contexts: a simple one for desktop/tablet/handheld, one for device type, one for OS, etc? Have to think about this.

That will be the building block for everything else. How we use the cache contexts will require some discussion.

Log in or register to post comments

Comment #12

darol100 commented 2 December 2015 at 17:38

Issue summary:	View changes
Related issues:		+#2627482: Integration with Composer, +#2627504: Cache Support For Mobile_detect, +#2627514: Create a service to load the Mobile_Detect library

1 file was hidden/shown/deleted

Status	File	Size
hidden	2443329-experimental-code-for-mobile_detect_d8.patch	9.08 KB

@mpdonadio, I open 3 issues base on #11 so we can fixed them individual instead of everything at once in this big issue.

I will take care of #2627482: Integration with Composer.

Log in or register to post comments

Comment #13

darol100 commented 5 December 2015 at 02:01

Issue summary:	View changes
Related issues:		+#2629304: Mobile_Detect - hook_requirements Drupal 8 Port, +#2629040: Mobile_Detect - hook_block_info and hook_block_view Drupal 8 Port, +#2629024: Mobile_Detect - hook_permission Drupal 8 Port, +#2629014: Mobile_Detect - README.txt Drupal 8 Port, +#2629012: Mobile_Detect - hook_help Drupal 8 Port

Scanning into the D7 version of this project, I open few issues to that needs to be port it. Adding them as relate issue and in the summary of this meta issue.

Log in or register to post comments

Comment #14

hongpong commented 18 July 2016 at 19:25

take a look at : https://github.com/tabbabi/detect_mobile done 2 years ago

Log in or register to post comments

Comment #15

Newb_Druper commented 18 January 2017 at 00:49

At comment #14, I tried that link and it works but brings the same old issue that D7 version original had of page caching which has been resolved by adding more code to the settings.php.

In other words it is caching the first instance of the page and serving that same page regardless if user is on mobile or desktop. As you can see it is still an open issue https://github.com/tabbabi/detect_mobile/issues/3

Log in or register to post comments

Comment #16

prashant.c

he/him

English

Dharamshala

commented 17 November 2017 at 09:46

I tested the module created here https://github.com/tabbabi/detect_mobile only local and it seems to be working fine and detecting the device properly.

Why don't we use this as Drupal 8 version of this module ?

Log in or register to post comments

Comment #17

prashant.c

he/him

English

Dharamshala

commented 17 November 2017 at 09:49

Only issue is with the caching when used in TWIG files. Due to caching it is sometimes not providing the correct info about device type.
If this would be fixed then this would be good to go.

Log in or register to post comments

Comment #18

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 17 November 2017 at 13:33

#16, create an issue and post a patch against 8.x-2.x. I did a very quick look, and this could be a solution.

Log in or register to post comments

Comment #19

hongpong commented 17 November 2017 at 17:00

Status	File	Size
new	tabbabi-2443329-19.patch	318 bytes

[wrong patch file , one sec]
Hi mpdonadio prashant.c, darol100, Newb_Druper, I have made a patch covering this. 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.

Log in or register to post comments

Comment #20

hongpong commented 17 November 2017 at 17:03

Status:

Active

» Needs work

Status	File	Size
new	tabbabi-20-2443329.patch	77.65 KB

Here is the patch. setting 'needs work' because of third party library.

Log in or register to post comments

Comment #21

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 17 November 2017 at 17:14

Status:

Needs work

» Active

#20, can you make a new issue with the patch? That way we can keep the review there. This is really just a Plan issue to talk about things a not for real work. Thanks.