Page MenuHomePhabricator

Fix support for pk-zip compressed figlet font files
ClosedPublic

Authored by cspeckmim on Apr 25 2023, 3:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 23, 6:14 PM
Unknown Object (File)
Tue, Mar 19, 8:42 PM
Unknown Object (File)
Thu, Mar 14, 5:58 PM
Unknown Object (File)
Sun, Mar 10, 5:37 PM
Unknown Object (File)
Sun, Mar 10, 3:14 PM
Unknown Object (File)
Thu, Feb 29, 8:07 AM
Unknown Object (File)
Feb 20 2024, 11:58 AM
Unknown Object (File)
Feb 19 2024, 12:07 AM
Subscribers

Details

Summary

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.

Test Plan
  1. 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.
  2. 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

Repository
rP Phabricator
Branch
cspeck-php8-figlet
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 25774
Build 35602: arc lint + arc unit

Event Timeline

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.

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
125–126

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.

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?

Okay I figured out what was happening.

  1. We observed the issue with using {} as index operator on our production server
  2. I updated the Figlet.php on our production server to fix the indexing issue
  3. This resulted in getting the zip_open issue, which this error result was cached (return raiseError() vs. a 500 php error)
  4. 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
  5. 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.
  6. 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.

Fix the implementation of using pk-zip compressed figlet font files

I am messing something up with git and arc?

cspeckmim retitled this revision from Remove support for compressed figlet font files. to Fix support for pk-zip compressed figlet font files.Apr 25 2023, 7:34 PM
cspeckmim edited the summary of this revision. (Show Details)
cspeckmim edited the test plan for this revision. (Show Details)

Aha! Looks good, then. Thanks!

This revision is now accepted and ready to land.Apr 25 2023, 8:08 PM