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

Remove gears detection #15627

Closed
wants to merge 17 commits into from
Closed

Remove gears detection #15627

wants to merge 17 commits into from

Conversation

pebosi
Copy link
Contributor

@pebosi pebosi commented Feb 26, 2020

Removing detection of old Gears plugin, was developed by Google. It was stopped in 2011: https://gearsblog.blogspot.com/2011/03/stopping-gears.html

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, development of Gears has been discontinued a while back already and newer browser don't have that anymore. On matomo.org there had been 8 visits with Gears enabled last year. Wondering if it would be still worth to keep old data 🤔

ping @tsteur @mattab

tests/resources/piwik-1.13-dump.sql Outdated Show resolved Hide resolved
plugins/DevicePlugins/Updates/4.0.0.php Outdated Show resolved Hide resolved
core/Updates/4.0.0-b1.php Outdated Show resolved Hide resolved
tests/javascript/frameworks/dojo/dojo-1.0.3.js Outdated Show resolved Hide resolved
tests/javascript/frameworks/dojo/dojo-1.1.2.js Outdated Show resolved Hide resolved
tests/resources/piwik-1.13-dump.sql Outdated Show resolved Hide resolved
@Findus23
Copy link
Member

Findus23 commented Mar 2, 2020

While we are on it:

Should we maybe also remove Quicktime, Realplayer and Director (which I think is the shockwave player)?

All of those shouldn't work in any modern browser anymore (and as the report notes, detection doesn't work in IE, so there should be nothing left)

@pebosi pebosi requested a review from sgiehl March 3, 2020 15:18
@pebosi
Copy link
Contributor Author

pebosi commented Mar 4, 2020

While we are on it:

Should we maybe also remove Quicktime, Realplayer and Director (which I think is the shockwave player)?

All of those shouldn't work in any modern browser anymore (and as the report notes, detection doesn't work in IE, so there should be nothing left)

i would create a pull request per Plugin

@pebosi pebosi requested a review from sgiehl March 4, 2020 13:32
@sgiehl
Copy link
Member

sgiehl commented Mar 4, 2020

code changes look fine so far. Need to find some time to check it out locally and do some testing...

@sgiehl sgiehl added this to the 4.0.0 milestone Mar 4, 2020
@sgiehl sgiehl added the Needs Review PRs that need a code review label Mar 4, 2020
@mattab
Copy link
Member

mattab commented Mar 5, 2020

Should we maybe also remove Quicktime, Realplayer and Director (which I think is the shockwave player)?

Probably a good idea, any thoughts @sgiehl @tsteur ?

@tsteur
Copy link
Member

tsteur commented Mar 5, 2020

I suppose should be fine. Not sure though re removing that data. You're basically removing historical data there of users. It may be fine as likely nobody looks at this anymore, but also feels a bit wrong.

@sgiehl
Copy link
Member

sgiehl commented Mar 6, 2020

Looking at the failing tests it seems all fingerprints changed due to the changes here. While that is fine for the tests and we can simply update them. I'm wondering if that is something we should adjust actually, as fingerprints for already tracked users might change after an update and they might be detected as new users then 🤔

Maybe we should use a default value 0 for gears in the config hash instead of removing it here:
https://github.com/matomo-org/matomo/pull/15627/files#diff-02edccc593e3badd1e9fd8d278fd1d35L113

Or is changing the fingerprints fine? (ping @mattab @tsteur )

@mattab
Copy link
Member

mattab commented Mar 10, 2020

Not sure though re removing that data. You're basically removing historical data there of users. It may be fine as likely nobody looks at this anymore, but also feels a bit wrong.

Let's at least mention this clearly in the Developer Changelog? (in a section "Database Schema" or so?)

Maybe we should use a default value 0 for gears in the config hash instead of removing it here:

👍

core/Tracker/Settings.php Outdated Show resolved Hide resolved
Use 0 as default

Co-Authored-By: Stefan Giehl <stefan@matomo.org>
@tsteur
Copy link
Member

tsteur commented May 12, 2020

@pebosi @sgiehl is there much left to do here?

@pebosi
Copy link
Contributor Author

pebosi commented May 12, 2020

I thought not.

@tsteur
Copy link
Member

tsteur commented May 12, 2020

👍 be great to get this merged @sgiehl not sure if something is missing

@sgiehl
Copy link
Member

sgiehl commented May 15, 2020

I'll recreate this PR with solved merge conflicts and fixed tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants