Test a glyph's membership of a GDEF class by the glyph, not by its hex turning up in the class (#193)

[jakejackson1]

Closes #193

What

A GDEF class is stored as " 00300| 00301", and every reader checked whether a glyph was in one with strpos. GlyphString::of() writes a plane 16 character six digits wide, and a six-digit glyph contains two five-digit ones: U+100300 is 10030 + 0, and 1 + 00300. So if a class named U+100300, both U+0300 and U+10030 counted as members. The issue only mentions the first case. The second one (a prefix) gives the same false match.

The stored formats don't change. They're still cached in tmp/ttfontdata and pinned by the fontcache master. GlyphString gets two helpers:

• set($glyphs): a "|"-separated list, with or without GDEF's leading spaces, as hex => true. Use it for a list that's checked glyph after glyph.
• inList($glyphs, $glyph): true only where no hex digit touches the glyph on either side. Use it for a list that's checked too rarely to be worth building a set. Any separator works, so one function handles " 00300| 00301", "00300|00301", finals' "0FE8E 0FE94 " and the ignore pattern "((?:(?: 00300| 00301))*)".

The sites use them like this:

• LookupFlag::skips() looks each class up in a set, built once per class (and per attachment class or filtering set).
• Otl keeps one LookupFlag per font key rather than building one for every run, and uses the mark set that LookupFlag::marks() hands out. Its 20 strpos($this->GlyphClassMarks, …) checks become isMark(), and LineBreaking::southEastAsian() takes the set.
• The MarkToBase, MarkToLigature and MarkToMark subtables used to find the base with strpos over a "|"-joined Coverage string and take its Coverage Index as strpos(...) / 6. They now look up both in a cached hex => Coverage Index map (coverageIndexByHex()), built straight from the table. That removes _getCoverage(), whose only remaining job was feeding this. The / 6 relied on the same fixed width, and was out by a fraction wherever a longer glyph came before the base.
• finals, Shaper\Arabic's backtrack/lookahead (prel/postl) and ignore strings, TTFontFile::gsubContextRule()'s check of a nested lookup's first glyph against the rule position, and OtlDump::positionHolds() go through inList().

No parsed font can reach this today

TTFontFile skips every cmap format 12 entry at or above U+30000 (if ($unichar < 0x30000), TTFontFile.php:1060). With OTL on, it then maps any glyph that only such an entry reaches into the Private Use Area (U+E000…, then U+2CEB0…). So every glyph in every class, Coverage string and rule the parser writes is exactly five digits wide, and a five-digit strpos can only match a whole token. The synthetic font below shows this: GDEF classes u100300 as a mark, and the parser caches that as 0E001. Upstream has the same cap (TTFontFile.php:1085), so the bug is latent there too.

The false match does happen wherever a class holds a six-digit glyph: LookupFlag fed its classes directly, as in the issue's example, or the shaper handed GDEF as the font states it. It would also happen if the U+30000 cap were ever lifted. The tests below cover both of those paths.

Inventory

Every strpos/stripos/preg_match/in_array in src/ checked against a glyph string. Line numbers are from 37f1e92. "Can false-match" means a five-digit glyph found inside a six-digit token, or the reverse.

Site	List format	Can false-match?	Change
LookupFlag::skips() :150	GDEF class, marks outside a filtering set or attachment class	yes (suffix and prefix)	set per class
Otl 20 × strpos($this->GlyphClassMarks, …): :352, :616, :642, :723, :1021, :1048, :1201, :1231, :1243, :1273, :2739, :2809, :2903, :2928, :3129, :3192, :3196, :3566, :3571, :3652	GDEF marks	yes	isMark(), set per font
Otl :1093, :2825	same	commented-out code	left alone
Otl::_applyGPOSmarkToBase/Ligature/Mark :3575/:3589, :3656/:3670, :3757/:3771	`"	"-joined Coverage, index by / 6`	yes, and the index shifts
Otl :667 finals	"0FE8E 0FE94 "	yes	inList()
Otl :669, :1811; TTFontFile :2216, :2280, :2290, :2309, :2317	literal BMP lists ('0FEAE 0FEF0 0FEF2', the viramas)	no: every token is five digits, so a five-digit needle only matches a whole token and a six-digit one matches nothing	none
Shaper\LineBreaking::wordMatch() :146, :153, :167	GDEF marks	yes	set, passed in by Otl
Shaper\Arabic::shape()	GDEF marks, `explode('	')+hexdec`	no, already a set
Shaper\Arabic::glyphs() :309, :310, :315, :326, :327, :332	prel/postl (`"	"-joined), ignore(the_getGSUBignoreString` pattern)	yes
TTFontFile::gsubContextRule() :2770	rule position, `"	"`-joined	yes
OtlDump::positionHolds() :1626, :1629	`"	"-joined coverage / class0excl`	yes
OtlDump::formatEntity*() :1932, :1975, :2044, :2064	GDEF marks	yes	out of scope here: #194 replaces them with isMark()
Otl::getGCOMignoreSet(), checkContextMatchMultiple(), _getClasses(), getCoverageUni(); GSLuCoverage/LuCoverage	already keyed sets / maps	no	none
TTFontFile::addPuaGlyphs() :1496 preg_match('/(0[EF][A-F0-9]{3})/')	pulls a glyph out of a token, not a membership test	would read 10E123 as 0E123, but unreachable for the same reason as above	none
Otl :1179, :1360–:1376, :1471–:1622; Shaper\OtlTags; Arabic :256	feature, script and language tags	not glyphs	none

The ignore patterns

_getGSUBignoreString() builds ((?:(?: 00300| 00301))*), but nothing in src/ runs these as regexes any more. The match/matchback patterns stay inside _getGSUBarray(): _getGSUBtables() only strips their parentheses to get keys. The shaper matches contexts against sets. The one place an ignore string gets past the parser is rtlSUB[…]['ignore'], and Shaper\Arabic::glyphs() searches it with strpos. That search is fixed through inList(), so no pattern needed a lookahead and the cached pattern text doesn't change. For the record, a leading space doesn't make a regex search safe: 10030 matches the start of 100300.

Test font

NotoSans-PlaneSixteenMark-Synthetic.ttf (2,608 bytes) is Noto Sans 2.007 (OFL 1.1), built in fontTools 4.59.2:

• Cut down to space, A, grave, asciitilde, gravecomb and acutecomb.
• Two glyphs added: u10030 (drawn as A, mapped to U+10030) and u100300 (drawn as gravecomb, mapped to U+100300), with cmap formats 4 and 12.
• GDEF, GSUB and GPOS replaced, and name IDs 1, 4 and 6 renamed.
• GDEF classes gravecomb and u10030 as bases, and acutecomb and u100300 as marks.
• ccmp: one lookup with lookupflag IgnoreMarks that substitutes grave for gravecomb.
• mark: MarkToBase that attaches acutecomb to u10030.

GdefClassMembershipTest builds an Otl over the font's cache, as KashidaFinalFormTest does:

• It asserts the parser's cached marks are 00301| 0E001, which documents the remap.
• It then gives the shaper GDEF as the font states it (marks 00301| 100300) and checks each glyph's codepoint, group and positioning:

Run	Before	After	hb-shape 14.3.1
U+0041 U+0300	0041 C, 0300 M: not substituted, and grouped as a mark	0041 C, 0060 C	A grave
U+10030 U+0301	mark not attached	mark at XPlacement 300, BaseWidth 639	u10030, acutecomb@-339,0 (300 − 639)

Unit tests:

• GlyphStringTest: set() and inList() for plane 16 against a BMP suffix and a plane 1 prefix, the first and last of several entries, an entry between two that contain it, an empty class, a Coverage string without spaces, and the finals and ignore-pattern formats.
• LookupFlagTest: the issue's example exactly. skips(IGNORE_MARKS, '00300') is false, '00301' is false, '100300' is true.
• ArabicTest: a prel naming 100628 isn't satisfied by BEH 00628, and an ignore naming 10064E doesn't skip FATHA 0064E.
• LineBreakingTest: a word doesn't end before a mark in the set, and a character whose hex is only part of a mark's (00E2A inside 100E2A) isn't treated as one.

Fixtures

Regenerated from cold: I deleted tests/Mpdf/tmp/mpdf, tmp/mpdf and tmp/ttfontdata, then ran composer fontcache:update all, otldump:update all, shaping:update all and subset:update all. git status showed only the new font's five files. No existing fixture moved. That's expected: every glyph in the corpus is five digits, so the old substring checks and the new exact ones agree everywhere.

The new font's masters:

• fontcache: GlyphClassMarks is 00301| 0E001, the remapped mark.
• otldump: the ccmp lookup reports Ignoring: Mark Glyphs and U+0300 » U+0060. The mark lookup reports bases 𐀰.
• shaping and subset: nothing unusual. The shaping master's runs are Latin and other scripts the font has no glyphs for.

Mutation check

For each site class, I put the substring test back, cleared the caches and ran the full suite.

Mutated	Fails
LookupFlag::skips() → strpos	LookupFlagTest::testAGlyphWhoseHexEndsAPlaneSixteenMarksIsNotSkippedWithIt; GdefClassMembershipTest "IgnoreMarks does not skip U+0300…"
Otl::isMark() → strpos($this->GlyphClassMarks, …)	both GdefClassMembershipTest runs (the U+0300 run fails on its group, M)
LineBreaking → strpos over the marks	LineBreakingTest::testAWordDoesNotEndBeforeAMark
Shaper\Arabic → strpos	both ArabicTest::testAFormsContextIsNotMetByAGlyphWhoseHexIsPartOfOneItNames runs
GlyphString::inList() boundary check removed	5 GlyphStringTest cases
GlyphString::set() also keying partial hex	5 GlyphStringTest cases, LookupFlagTest
Otl finals → strpos	nothing
Otl::coverageIndexByHex() substring lookup	nothing
TTFontFile::gsubContextRule() → strpos	nothing
OtlDump::positionHolds() → strpos	nothing

The last four only get six-digit glyphs from lists that come out of the parser (finals, rule positions, Coverage), and the parser can't produce them (see above). Only the GlyphString::inList() unit test covers them. coverageIndexByHex() is keyed from _getCoverage(), which is capped the same way, so nothing covers it beyond reading the code.

Performance

WriteHTML of about 60 repeats of a paragraph per font, warm font cache, best of 7, three alternating rounds against a 37f1e92 checkout (PHP 8.5.4):

Font / text	37f1e92	this PR
DejaVu Sans, Latin with stacked marks + Vietnamese + Cyrillic	81–88 ms	60–69 ms
FreeSerif, Devanagari	293–320 ms	209–241 ms
XB Riyaz, pointed Arabic	170–191 ms	160–195 ms
Garuda, Thai with dictionary line breaking	85–90 ms	84–105 ms
Jomolhari, Tibetan	193–219 ms	195–222 ms

DejaVu Sans and FreeSerif, whose GDEF mark classes are large, got about 25% faster. The other three are within noise.

Mapping onto upstream

mpdf/mpdf development (744f75a) has no LookupFlag, GlyphString, Shaper\* or isMark(), and it still matches contexts against strings. It has the same cap on cmap (TTFontFile.php:1085), so the bug is latent there too. The checks to change:

• Otl::_checkGCOMignore() (:4529), TTFontFile::_checkGSUBignore() (:2928) and OtlDump::_checkGSUBignore() (:2688): five strpos checks each, against GlyphClassMarks, GlyphClassLigatures, GlyphClassBases, MarkAttachmentType[...] and MarkGlyphSets[...].
• Otl: 24 active strpos($this->GlyphClassMarks, …) checks (:211 … :3602), three of them in checkwordmatch() (:3113–:3134).
• Otl: strpos($ignore, …) in the GPOS and context walkers (:1797, :3357, :3409, :3695, :4181–:4347), and strpos($Input[$i] / $Backtrack[$i] / $Lookahead[$i] / $class0excl …) in checkContextMatchMultiple() (:4248–:4288). The fork replaced these with sets in earlier PRs.
• Otl: finals (:506), get_arab_glyphs() (:2956–:2979), and $BaseGlyphs / $LigatureGlyphs / $Mark2Glyphs with / 6 (:3534–:3712).
• TTFontFile::_getGSUBarray(): strpos($inputGlyphs[$seqIndex], $lookupGlyphs[0]) (:2508, :2571, :2639, :2718, :2834, :2901).
• OtlDump: strpos($coverage, …) (:2018–:2116), strpos($lcoverage, …) (:3276–:3725), and the four formatEntity* checks (:4262–:4362), which Report only the substitutions the parser keeps, and draw only GDEF's marks on a dotted circle (#187) #194 covers here.

The upstream PR could add GlyphString::set()/inList(), or a trailing-delimiter check (strpos($class . '|', ' ' . $glyph . '|')) at each site. The unit cases in GlyphStringTest carry over. GdefClassMembershipTest needs the fork's Otl construction seam, so upstream would test through _checkGCOMignore() instead. The upstream PR also needs a CHANGELOG.md entry.

/simplify

Applied:

• Otl and LookupFlag each built their own set of GDEF's marks. LookupFlag::marks() now hands out the one it builds, and Otl keeps only the LookupFlag per font, so $gdefSets is gone.
• marksOutsideFilteringSet() reads the mark glyph set through GlyphString::set() instead of its own explode/trim.
• coverageIndexByHex() reads the Coverage table directly, and _getCoverage() and its cache, which nothing else read, are gone.

After these changes, the suite, snapshots, cs and phpstan all match the numbers below, the golden masters regenerated from cold are unchanged, and the LookupFlag and isMark() mutations still fail the same tests.

Skipped:

• Writing inList() as one preg_match with lookarounds. That compiles a new pattern for every glyph asked about, and the loop is a single strpos on a miss.
• Caching the list of sets to test per flag in skips(). The per-call setup (skipped(), checkMarkFilteringSet()) is what it already did before this PR.
• Passing the set into Shaper\Arabic::shape(). It already builds its own codepoint-keyed set correctly, and its public signature takes the string.
• Stashing the LookupFlag inside GDEFdata. GDEFdata is the cached array the shaper reads back, and a separate property keeps objects out of it.
• Dropping GdefClassMembershipTest's parser test. It records why the other tests hand the shaper GDEF directly.

Also found

Shaper\Arabic::glyphs() loops forever on PHP 8 when a form's backtrack or lookahead is walked past the edge of the run over ignored glyphs. $chars[-1] is null, and strpos($ignore, null) returns 0. This happens before and after this PR, so it's filed separately as #204 rather than fixed here.

Base

Stacked on #198 (refactor/160-otl-tenants) → #190 → #188 → #186 → gravitypdf. Before opening, I checked all four with gh pr view: all are open and none are merged, and refactor/160-otl-tenants is still at 37f1e92. So this PR targets it with no rebase. It doesn't conflict with #194 (this PR only touches positionHolds() in OtlDump) or #185 (this PR doesn't touch LookupFlag::skipped()).

Verification

• vendor/bin/phpunit: 2078 tests, 5955 assertions, 1 skipped. The baseline was 2053 / 5904 / 1. The extra tests are the new font's four golden-master data sets plus the new unit and shaping tests.
• vendor/bin/phpunit --group=snapshot: OK, 79 tests, 110 assertions, none skipped.
• composer cs: clean.
• phpstan: 33 errors, the same as the baseline, and none in touched files.

🤖 Generated with Claude Code

[jakejackson1]

Rebased: #190 was squash-merged into gravitypdf (f9b3c0e), so this stack still carried its pre-squash commits. This branch is rebased onto its rebased parent with --onto; no conflicts and the base is unchanged. Suite, snapshots and cs pass on the new head, with no fixture movement.

[jakejackson1]

This PR merged into a base branch that had already been squash-merged, so its change never reached gravitypdf. It's carried to gravitypdf by #217.