search

owncloud / core

:cloud: ownCloud web server core (Files, DAV, etc.)
https://owncloud.com
GNU Affero General Public License v3.0
8.36k stars 2.06k forks source link

Drop jquery-migrate v1 #12877

Closed MorrisJobke closed 5 years ago

MorrisJobke commented 9 years ago
MorrisJobke commented 9 years ago

Look at #13646 for a dependency, that needs to be resolved in advance.

LukasReschke commented 9 years ago

Adding security label due to http://research.insecurelabs.org/jquery/test/

DeepDiver1975 commented 9 years ago

@LukasReschke 8.2? 9.0?

MorrisJobke commented 9 years ago

@LukasReschke 8.2? 9.0?

Has the potential to break a lot. We could drop it early in 9.0 and then see what happens.

MorrisJobke commented 9 years ago

Another idea was to only drop the methods, that has security problems. This would reduce the risk of broken apps/code.

LukasReschke commented 9 years ago

Let's try to drop this with 9.0 – patching this manually is igitt ibah :smile:

PVince81 commented 8 years ago

Tech debt and risky => 9.1

PVince81 commented 8 years ago

@ChristophWurst now that we upgraded to jquery 2.2, is it the right time to get rid of jquery-migrate ? (which you apparently also upgraded in the PR)

PVince81 commented 8 years ago

ref https://github.com/owncloud/core/pull/23993

ChristophWurst commented 8 years ago

@PVince81 when removing jquery-migrate we'd have to fix all migration issues and follow jQuery's upgrade guide. Enabling jq-migrate warning as @MorrisJobke suggested above will help detect code that needs to be changed. We should migrate to the new jQuery APIs, but I assume it will take some time to find the files/functions that need to be touched as the warning is only shown if you actually use a deprecated function. Once core is fully migrated to jQuery 2.x only, we can remove jquery-migrate. However, this will potentially break some apps.

ChristophWurst commented 8 years ago

Running js tests with the non-minified jquery-migrate version throws the following warning:

WARN: 'JQMIGRATE: jQuery.browser is deprecated'
WARN: 'JQMIGRATE: jQuery.fn.unload() is deprecated'
WARN: 'JQMIGRATE: jQuery.parseJSON requires a valid JSON string'
WARN: 'JQMIGRATE: jQuery.fn.attr('checked') might use property instead of attribute'
PVince81 commented 7 years ago

moving to backlog for now

cc @felixheidecke FYI

PVince81 commented 6 years ago

@felixheidecke

Some migration was done already, need to check the upgrade guide to find what functions to grep for through all apps.

felixheidecke commented 6 years ago

Core works just fine without jQuery Migrate. OC.SetupChecks doesn't return errors if the headers are missing, this needs some looking into.

felixheidecke commented 6 years ago

Checking the Enterprise bundle 10.0.8 agains changes and deprecations in jQuery 1.9 and 2.x

Removed

.browser()

.live()

.die()

.sub()

.andSelf()

Changed

.toggle()

.add()

.after()

.before()

.replaceWith()

.appendTo()

.insertBefore()

.insertAfter()

.replaceAll()

.ajax()

Minor

.trigger()

.click()

.focus()

Neglectable

.data()

.parents()

.focus()

.attr()

.prop()

PVince81 commented 6 years ago

@felixheidecke as discussed yesterday, next steps:

felixheidecke commented 6 years ago

See the script here: https://gist.github.com/felixheidecke/c32ab1b38f7091f5c5d18d4a24fd0024

felixheidecke commented 6 years ago

Please use this core branch to test against https://github.com/owncloud/core/tree/jquery-2-migration

PVince81 commented 6 years ago

@felixheidecke I can't login with the test branch. Seems something broken in the login page, can you debug ?

PVince81 commented 6 years ago

Never mind, seems I had an older unsupported app "files_odfviewer" enabled which fails due to missing .live() method and messes with the login page JS. After disabling the app it works again.

felixheidecke commented 5 years ago

Can Sharepoint app check be assigned to someone with Sharepoint testing abilities?

PVince81 commented 5 years ago

@felixheidecke apart from sharepoint, are we done ? close ?

felixheidecke commented 5 years ago

We are done indeed