The change from https://secure.phabricator.com/D21860 introduced a PHP error due to an invalid variable reference (zip instead of $zip). This fixes that issue as well as confirms that pk-zip compressed figlet font files can continue to be used/loaded.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- rPbc6f4786a2e3: Fix support for pk-zip compressed figlet font files
- I ensured I had numerous figlet font files installed in resources/figlet/custom and used file to verify that univers.flf and puffy.flf are pk-zip archives.
- With this change applied I added a comment with figlet(font=univers){{{hello}}} and verified that the comment used the univers font to render hello, and like-wise with the puffy font.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Sorry for jumping the gun on landing that previous one. To my surprise we didn't even have any custom figlet font files installed previously. I did some more thorough testing with this change to ensure that custom figlet font files still work properly, unless they're compressed. I'm not sure why the change D21860 did not work to decompress the tar/gzip figlet file but if it's not worth supporting I prefer to remove the complexity.
Comment Actions
I'm not sure why the change D21860 did not work to decompress the tar/gzip figlet file
Looking more into the ZipArchive documentation it looks like I would've had to re-assign $fp to $zip->getStreamIndex(0); in order for the rest of the code to read data from the zip entry.
| externals/pear-figlet/Text/Figlet.php | ||
|---|---|---|
| 150 | Clearly wasn't hitting this during runtime. I was mixing up .tar.gz with .zip archives. Even testing out changes to reassign $fp to $zip->getStreamIndex(0); and using appropriate pkzip I haven't been able to get this to work. | |
Comment Actions
I'm a little confused about how we ever hit the test for zip_open() (or whatever prompted the original change in D21860) if you don't have any custom compressed fonts and Phabricator doesn't ship with any custom compressed fonts? This change looks good to me, but how did we run into a problem in the first place?
Comment Actions
Okay I figured out what was happening.
- We observed the issue with using {} as index operator on our production server
- I updated the Figlet.php on our production server to fix the indexing issue
- This resulted in getting the zip_open issue, which this error result was cached (return raiseError() vs. a 500 php error)
- I applied the index issue change and the zip_open change to our test environment and was able to load the diff that was failing in production
- Someone had previously installed a bunch of figlet fonts on our production server but it was after the last time we duplicated production to our test server - so the tests I was running in our test environment never hit the zip issue directly.
- On the production server many of the fonts are PK-zip compressed however have the .flf extension. I had been looking for .zip or .tar.gz files so I had assumed none of the fonts installed were compressed.
That said, I did confirm an update to still support the PK-zip archives using ZipArchive api, using pk-compressed flf font files. I'll update this with the changes.