Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix #928 : make compatible to extends #936

Merged
merged 1 commit into from
Aug 11, 2023
Merged

Conversation

pquerner
Copy link

@pquerner pquerner commented Aug 8, 2023

Fixes #928

@serbanghita serbanghita self-requested a review August 9, 2023 08:49
@serbanghita serbanghita merged commit ba9281b into serbanghita:2.8.x Aug 11, 2023
@serbanghita
Copy link
Owner

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

@pquerner
Copy link
Author

I can't.
#928 (comment) ^^

Not yet anyways.

@francislavoie
Copy link

I think this broke Agent: jenssegers/agent#208

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

@pquerner
Copy link
Author

pquerner commented Nov 6, 2023

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

@francislavoie
Copy link

francislavoie commented Nov 6, 2023

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
Copy link
Author

pquerner commented Nov 6, 2023

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
Copy link

See jenssegers/agent#208 for the explanation.

@pquerner
Copy link
Author

pquerner commented Nov 6, 2023

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
Copy link

francislavoie commented Nov 6, 2023

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 #940 and that would give a good migration path for everyone using Agent, and removing a link in the dependency chain.

@serbanghita
Copy link
Owner

@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
Copy link

Sounds great 🫶

@pquerner
Copy link
Author

pquerner commented Nov 7, 2023

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
Copy link
Owner

Fixed, details at jenssegers/agent#207 (comment)

@francislavoie
Copy link

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

@pquerner pquerner deleted the 2.8.x branch November 8, 2023 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants