Page MenuHomePhabricator

When PHPExcel is not installed, detect it and provide install instructions
ClosedPublic

Authored by epriestley on Jan 29 2018, 3:57 PM.
Tags
None
Referenced Files
F13050336: D18958.id45482.diff
Fri, Apr 19, 2:46 AM
F13050335: D18958.id45470.diff
Fri, Apr 19, 2:46 AM
F13050334: D18958.id45457.diff
Fri, Apr 19, 2:46 AM
F13050332: D18958.id.diff
Fri, Apr 19, 2:46 AM
F13049872: D18958.diff
Fri, Apr 19, 2:29 AM
F13046253: D18958.id45457.diff
Thu, Apr 18, 8:08 AM
Unknown Object (File)
Tue, Apr 16, 5:27 AM
Unknown Object (File)
Mon, Apr 15, 11:33 AM
Subscribers

Details

Summary

Depends on D18957. Ref T13049. To do Excel exports, PHPExcel needs to be installed on the system somewhere.

This library is enormous (1K files, ~100K SLOC), which is why we don't just include it in externals/. This install process is a little weird and we could improve it, but users don't seem to have too much difficulty with it. This shouldn't be worse than the existing workflow in Maniphest, and I tried to make it at least slightly more clear.

Test Plan

Uninstalled PHPExcel, got it marked "Unavailable" and got reasonably-helpful-ish guidance on how to get it to work. Reinstalled, exported, got a sheet.

Diff Detail

Repository
rP Phabricator
Branch
export8
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/infrastructure/export/format/PhabricatorExcelExportFormat.php:17XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 19254
Build 26020: Run Core Tests
Build 26019: arc lint + arc unit

Event Timeline

amckinley added inline comments.
src/infrastructure/export/format/PhabricatorExcelExportFormat.php
36

This is in php.ini, right? Maybe make that explicit?

This revision is now accepted and ready to land.Jan 29 2018, 9:34 PM

To make this slightly easier, we could also create some phabricator/externals/clone-stuff-here/ directory and say "put it here", and then automatically add that (or, that plus PHPExcel/Classes, I guess) to include_path so you only had to do the git clone and didn't have to mess with php.ini. We could also raise a setup warning like "if you want excel support, cd into this directory and run git clone .... You can safely ignore this suggestion if you don't care about excel exports." to guide you down that path a little more smoothly.

To make this significantly easier we could ship PHPExcel in externals/ so it just works (or switch to PhpSpreadsheet and distribute that), but I'm really hesitant about bundling an extra 100K lines of externals. On the other hand, there's no really concrete cost to doing this, so maybe we should just bite the bullet and accept that we're distributing a spreadsheet exporting library with a little bit of code review functionality on top? (Phabricator is something in the realm of 1M lines, I think, so it's not quite that bad.)

Historically, I think users have had surprisingly little difficulty getting through this so it feels like improving this isn't terribly important. If more users were hitting issues with it I'd be more inclined to try to make it easier.

This revision was automatically updated to reflect the committed changes.

As per https://github.com/PHPOffice/PHPExcel/commit/c269793ee715fb5a15f0b2ad25c064d9ee6e8e53, PHPExcel has been deprecated and replaced with PHPSpreadsheet. So, instead of recommending PHPExcel, shouldn't PHPSpreadsheet be supported?

(I'd file a task for this, but that functionality isn't available anymore, so not sure where to mention this.)

(I added you to Community, so you should have sweeping janitorial powers on this install now.)