Page MenuHomePhabricator

Migrate Kartographer to Leaflet 1.x
Closed, ResolvedPublic

Description

When Mapbox JS client lib supports it, we should migrate to leaflet 1.x.

This is different from T147361, which is about updating the Kartotherian service's static resources.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Yurik renamed this task from Migrate to Leaflet 1.x to Migrate Kartographer to Leaflet 1.x.Oct 6 2016, 5:43 PM
Yurik updated the task description. (Show Details)

Change 317546 had a related patch set uploaded (by JGirault):
WIP: Upgrade mapbox.js to v3.0 (upgrade leaflet to 1.0)

https://gerrit.wikimedia.org/r/317546

@MaxSem any idea how to fix the failing phpunit tests?

https://integration.wikimedia.org/ci/job/mwext-testextension-hhvm-jessie/557/console

22:07:51 There was 1 failure:
22:07:51 
22:07:51 1) ResourcesTest::testFileExistence with data set #602 ('/home/jenkins/workspace/mwext...lt#VML', 'mapbox', '/home/jenkins/workspace/mwext...lt#VML')
22:07:51 File '/home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/extensions/Kartographer/lib/mapbox/#default#VML' referenced by 'mapbox' must exist.
22:07:51 Failed asserting that file "/home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/extensions/Kartographer/lib/mapbox/#default#VML" exists.
22:07:51 
22:07:51 /home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/tests/phpunit/structure/ResourcesTest.php:23
22:07:51 /home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/tests/phpunit/MediaWikiTestCase.php:400
22:07:51 /home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/maintenance/doMaintenance.php:111

I think it fails because of this line shape.style.behavior = 'url(#default#VML)'; in mapbox-lib.js ....

Help appreciated 😝

Fix the core or patch Mapbox?

Fix the core or patch Mapbox?

That's no mapbox bug though.
I guess I'll have to fix the core... 😓

I mean even if there's no bug with MB, we could still alter it to avoid triggering the failure. LMK if my help needed with core:)

Tested after rebase, and it seems to me that all features work . We only need to make CI tests pass.

debt removed JGirault as the assignee of this task.Jun 6 2017, 7:53 PM
debt moved this task from Stalled/Waiting to Backlog on the Maps-Sprint board.
debt removed a project: Patch-For-Review.
debt added subscribers: JGirault, debt.

Moving to backlog until such time that we can take this up again.

Looks like there are only tests to fix - moving off the sprint board as the Discovery team won't be able to finish this work at this time.

Leaflet 1.1.0 is out now btw.

retested this and still seems to work. Testcases are now OK, I think we can merge this.

Checked with @debt before merging.

Change 317546 merged by jenkins-bot:
[mediawiki/extensions/Kartographer@master] Upgrade mapbox.js to v3.0.1 (upgrade leaflet to 1.0.2)

https://gerrit.wikimedia.org/r/317546

I recently implemented a leaflet+vega integration, which allows any vega graph to be drawn on top of a map. Once Vega 3 is reviewd (T172938), it can be added to Kartographer. This would solve many wikivoyage requests too - like having unlimited number of data points on a map.

debt moved this task from Backlog to Needs review on the Maps-Sprint board.
debt added subscribers: Gehel, Pnorman.

@Gehel and @Pnorman - can you take a quick look and see if this merged patch can be released into production?

TheDJ claimed this task.