serbanghita / Mobile-Detect

Mobile_Detect is a lightweight PHP class for detecting mobile devices (including tablets). It uses the User-Agent string combined with specific HTTP headers to detect the mobile environment.
http://mobiledetect.net
MIT License
10.52k stars 2.67k forks source link

fix #928 : make compatible to extends #936

Closed pquerner closed 1 year ago

pquerner commented 1 year ago

Fixes #928

serbanghita commented 1 year ago

@pquerner btw why are you using the 2.8.x branch, you should use 3.74.x

pquerner commented 1 year ago

I can't. https://github.com/serbanghita/Mobile-Detect/issues/928#issuecomment-1659259903 ^^

Not yet anyways.

francislavoie commented 10 months ago

I think this broke Agent: https://github.com/jenssegers/agent/pull/208

Everyone using Agent is forced to revert/pin to 2.8.41 to maintain ->isMobile() functionality.

pquerner commented 10 months ago

What is your suggested way of a fix? Sorry I didnt catch the BC with the getter.

francislavoie commented 10 months ago

The problem is static::getOperatingSystems() calls the overriden function from Agent which adds in desktop OSes to the list, and that list is used in Mobile-Detect to match mobile devices, so it makes all desktop OSes match as mobile.

IMO this should be reverted and continue using the static properties. This was a breaking API change and shouldn't have been done in a patch release (though 2.9.x would have also caused trouble because Agent requires ^2.7.6 which would include 2.9.x).

pquerner commented 10 months ago

I dont see that as a working solution. Maybe for your 3rd party library, but not in favor of extendability.

Can you guide me what agent did by overwriting this getOperatingSystems(), which to my knowledge didnt get used prior. (Meaning didnt get called)

francislavoie commented 10 months ago

See https://github.com/jenssegers/agent/pull/208 for the explanation.

pquerner commented 10 months ago

Yes, again: Agent has overwritten an unused method to extend some functionality in 3rd party code and now the getter is actually used in here its causing issues. Move that custom code (merging / removing of other UAs) to somewhere else.

Maybe we can find a way of altering the rules to-be-applied to some generic place?

francislavoie commented 10 months ago

The problem is that lots of people use Agent (most of the Laravel community, and some others) and Agent is no longer maintained. So making breaking updates to Mobile-Detect means it breaks it "forever" for those users, unless they fork and fix it for themselves. Or we wait for a "blessed" fork, but that's asking a lot.

I agree that the better solution would be a clean API, but the problem is lack of maintenance of Agent, and no alternatives for those users because it adds additional functionality that is must-have for a lot of people.

I suggested Mobile-Detect ports the functionality from Agent in https://github.com/serbanghita/Mobile-Detect/issues/940 and that would give a good migration path for everyone using Agent, and removing a link in the dependency chain.

serbanghita commented 10 months ago

@francislavoie Okay, I'm reading this in details tonight, meanwhile:

shot term: will revert this medium term: will provide Mobile Detect with tokenization, version support for model, browser, os, build, etc + will update https://github.com/jenssegers/agent to a major version to support the changes

wdyt?

francislavoie commented 10 months ago

Sounds great 🫶

pquerner commented 10 months ago

Why is the version not pinned at "Agent"? Even more crucial when its abandoned. Sounds very silly to me all this. ^^ But yea, if short term revert solves all your problems.. ;D

Just hope someone has time to work on mobile-detect to bring all those features. Even to the very old 2.x branch.

serbanghita commented 10 months ago

Fixed, details at https://github.com/jenssegers/agent/issues/207#issuecomment-1800253853

francislavoie commented 10 months ago

Thanks, confirmed that ->isDesktop() now works correctly with 2.8.45