Paths

Table of Contentst

Differential D25142

Update Figlet implementation to be PHP8 compatible
ClosedPublic
Actions

Authored by speck on Apr 28 2023, 19:38.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15289: Update Figlet implementation to be PHP8 compatible
T15064: Make Phorge compatible with PHP 8.1/8.2/8.3/8.4

Commits

rP71e4eee275f5: Update Figlet implementation to be PHP8 compatible

Summary

As of PHP ~v8 the zip_open and associated functions have been deprecated and
removed. The replacement is the ZipArchive API. This updates the figlet
implementation to use this API which has been present in PHP since 5.2.

Additionally in PHP 8 the use of squiggly brackets for indexing into arrays
is also deprecated. This updates to remove two uses of squiggly brackets and
replace with square brackets.

These two deprecations would result in being unable to load differential
revisions in which someone had commented using figlet remarkup.

Imported from:

https://secure.phabricator.com/rPd5c63c86e7e4e87d5f72b35b1bdb1e888aea49bc

https://secure.phabricator.com/rPbc6f4786a2e36441d17b765fde8e8e047840bc58

Closes T15289

Test Plan

Applied these changes to an install and loaded a revision that had comments
where someone utilized figlet remarkup. The revision loaded properly and the
figlet comment rendered properly.

Diff Detail

Repository

rP Phorge

Branch

figlet

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 283
Build 283: arc lint + arc unit

Event Timeline

valerio.bozzolan created this revision.Apr 28 2023, 19:38

Owners added a reviewer: O1: Blessed Committers.Apr 28 2023, 19:38

Herald added subscribers: Cigaryno, Matthew, tobiaswiese, speck. · View Herald TranscriptApr 28 2023, 19:38

valerio.bozzolan requested review of this revision.Apr 28 2023, 19:38

Harbormaster completed remote builds in B283: Diff 546.Apr 28 2023, 19:38

valerio.bozzolan foisted this revision upon speck.Apr 28 2023, 19:38

valerio.bozzolan added a reviewer: valerio.bozzolan.

valerio.bozzolan mentioned this in Code Differences between Phabricator and Phorge.Apr 28 2023, 19:40

Hi @speck :D Do you still love Phorge, isn't it? ihih

Thank you for your beautiful patch in upstream Phabricator. Hoping to be useful, here the same thing, imported in Phorge (yep it already have the fix for the $zip variable)

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 28 2023, 20:08

valerio.bozzolan added a task: T15289: Update Figlet implementation to be PHP8 compatible.

valerio.bozzolan added inline comments.Apr 28 2023, 20:27

externals/pear-figlet/Text/Figlet.php
146	IMPORTANT: It seems `ZipArchive::RDONLY` was introduced since PHP 7.4.3 https://www.php.net/manual/en/zip.constants.php#ziparchive.constants.rdonly
147	✅ The strict check is correct since it must return `true` on success https://www.php.net/manual/en/ziparchive.open.php

We have not migrated to using Phorge of Phabricator, so issues that crop up which are critical will be updates I submit upstream.

externals/pear-figlet/Text/Figlet.php
146	huh, good find, I didn't consider that the argument would not have also existed since `ZipArchive` was added in 5.2 Some things to consider PHP < 7.4 is out of support according to https://www.php.net/supported-versions.php Debian bullseye+ and Ubuntu 20.04+ package manager ships PHP 7.4+ Debian buster and Ubuntu 18.04 package manager ships PHP 7.2 Red Hat 8+ have PHP 7.4+ available in shipped packages (Red Hat 7 does not ship any PHP it looks like) Fedora project's EPEL registry has packages for 8.2 Rocky Linux appstream includes 7.4+

valerio.bozzolan added inline comments.Apr 29 2023, 07:25

externals/pear-figlet/Text/Figlet.php
146	Uhmm I'm not bold enough to raise PHP 7.4 as minimum version just for this exact line, since probably the workaround here is reasonable readable and minimal. Here an example ↑

do not introduce issue for PHP before 7.4 related to flag ZipArchive::RDONLY

Harbormaster completed remote builds in B285: Diff 548.Apr 29 2023, 08:59

clarify in the documentation that the zip extension is useful (wow, don't know why it was not mentioned before)

Harbormaster completed remote builds in B286: Diff 549.Apr 29 2023, 09:04

Thank you so much again, your patch is really nice and useful.

Hoping to save some time to you I've added a small check to do not introduce any regression for PHP before 7.4, and also added a small note in the documentation that was probably a good-faith omission from our predecessors, and not something introduced with this change.

sgtm

This revision is now accepted and ready to land.Apr 29 2023, 09:05

Thank you for making the update. I'll do some testing and likely submit this upstream as well.

externals/pear-figlet/Text/Figlet.php
146	Yeah that should be a separate discussion. I'll copy that comment into T15047.

speck added inline comments.Apr 29 2023, 13:27

src/docs/user/installation_guide.diviner
124 ↗	(On Diff #549)	Is zip a separate extension for PHP? It seems to be included in the default installation, and there are likely other default extensions that are depended on (maybe openssl)?

valerio.bozzolan added inline comments.Apr 29 2023, 21:22

src/docs/user/installation_guide.diviner
124 ↗	(On Diff #549)	Nice question. It seems Debian and Ubuntu ship that component as a separated package installable by `sudo apt install php-zip`. I think it was installed by default also in my installation since probably it was required by lot of other packages I use. https://packages.debian.org/stable/php-zip https://packages.ubuntu.com/focal/php-zip This is what it currently installs: https://packages.debian.org/bullseye/amd64/php7.4-zip/filelist Also it seems some people talk about that relationship between class `ZipArchive` and `php-zip` package: https://stackoverflow.com/q/45117510/3451846 Also it seems the same is in CentOS: https://stackoverflow.com/a/53562962/3451846 So since `php-gd` is mentioned is probably OK to also mention `php-zip` to avoid later surprises. At least in the optional section, since we have not a complete overview, seems reasonable

valerio.bozzolan mentioned this in D25153: Fix PHP 8.1 "strlen(null)" exception which blocks rendering the Projects page (and log alien values).May 1 2023, 08:37

aklapper mentioned this in rP4d88095f2a53: Fix PHP 8.1 "strlen(null)" exception which blocks rendering the Projects page….May 1 2023, 20:22

Do you have any practical tip about how to test this?

(Premising I already approved this, so green light from me)

For a basic test additional compressed figlet font files have to be installed. I think it’s under src/support/figlet or src/resources/figlet or similar - pretty sure there’s a readme file in the location indicating it goes there. Then in a comment use remarkup to use figlet and verify it’s using the font you installed. For the zip check presumably you’d need to update php.ini to disable the zip extension.

In D25142#6649, @speck wrote:

Then in a comment use remarkup to use figlet

How?

(I've read this before asking ↓)

https://we.phorge.it/book/phorge/article/remarkup/

Oh I didn't realize it wasn't documented.

figlet (font=font_name) {{{
 hello
}}}

where font_name is the name of the figlet font you want to use, and can be omitted. results in:

_ _ _ | |__ ___| | | ___ | '_ \ / _ \ | |/ _ \ | | | | __/ | | (_) | |_| |_|\___|_|_|\___/

valerio.bozzolan mentioned this in T15401: Make the Remarkup Help Page Extensible.May 18 2023, 20:59

aklapper mentioned this in D25234: Fix exception and error message rendering some markup matching figlet/cowsay regex.May 20 2023, 15:31

valerio.bozzolan added inline comments.May 23 2023, 12:58

src/docs/user/installation_guide.diviner
124 ↗	(On Diff #549)	If it's OK for you, feel free to land :)

speck added inline comments.May 23 2023, 20:15

src/docs/user/installation_guide.diviner
124 ↗	(On Diff #549)	I'm not sure if this is necessary. See `PhabricatorZipSetupCheck.php` which will create a new setup issue if the zip extension is not present.

valerio.bozzolan added inline comments.May 23 2023, 20:19

src/docs/user/installation_guide.diviner
124 ↗	(On Diff #549)	Uhm. Just to clarify: so if there is a setup check, this should be not documented. Right? If your answer is "yep" I will remove that above change. Sorry for that.

speck added inline comments.May 23 2023, 20:40

src/docs/user/installation_guide.diviner
124 ↗	(On Diff #549)	I don’t feel strongly either way. The setup check guides the user to instruct examples of what doesn’t work if it’s not installed, but doesn’t indicate it’s required.