Keep every font alias a package registers - #36
Open
jakejackson1 wants to merge 1 commit into
Open
Conversation
`initFontRegistry()` finishes by deduplicating each property it merged:
$this->$property = array_unique(array_merge($values, $this->$property));
That is right for five of the six. `backupSubsFont`, `BMPonly`,
`sans_fonts`, `serif_fonts` and `mono_fonts` are lists of font keys, and
a repeated key there is noise worth dropping.
`fonttrans` is not a list. It maps an alias to the font it resolves to,
keyed by alias, and `array_unique()` compares *values* — so two aliases
naming one font collapse to whichever appeared first, silently. A
package registering
['arial' => 'somefont', 'helvetica' => 'somefont']
ends up with `arial` alone, and `font-family: helvetica` stops resolving
for no reason the author can see. Aliasing several names onto one font
is the ordinary case for a metric-compatible substitute, so this is not
an edge.
Deduplicate the five lists as before and leave the map alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1
added a commit
to GravityPDF/gravity-pdf
that referenced
this pull request
Sep 7, 2026
mPDF learned about fonts two ways in 6.x: a hard-coded list of the core fonts the installer downloaded, and a glob of the uploads fonts directory run on every render, whose results were invisible to the Font Manager. Both are gone. `Registry` builds two package layers from one cached read of the font tables — the fonts bundled in the plugin, and every row — and hands them to mPDF as a font registry. `Helper_PDF::begin_pdf()` builds the whole font config from it: the registry itself, the default font, the document script and language, the language map, and `autoScriptToLang`, which is load-bearing rather than a preference — mPDF never substitutes Arabic or Indic glyphs, so a lang tag on the run is the only route to a font for those scripts. Because the same object answers `get_grouped_fonts()`, the settings dropdown can no longer offer a font that will not render. It used to list the 6.x core fonts unconditionally, whether or not the installer had ever been run. `Loose_Font_Importer` replaces the render-time glob, and ships in the same change that removes it so no render loses a font it used to find. Each loose file becomes a row under the key 6.x derived from its filename, so existing templates keep resolving; a file that will not parse is skipped and logged rather than fatal, which is what it was before on the first render that selected it. The core-font installer's admin nag goes with them: its one-time-action route, its condition and redirect, its view and template, and the Tools-tab button. The React bundle that button mounted is now unreachable, but it is left for the removal sweep in the next phase rather than widened into this one. `tools/mu-plugins/mpdf.php` is deleted too. It forced mPDF into core-fonts mode for the whole test suite, so no test had ever proved a real font was embedded. `Test_Bundled_Render` replaces it and asserts what it was hiding: `onlyCoreFonts` false, all four Arimo faces loaded for bold and italic, the consent ticks coming from the symbol supplement, an unknown font-family falling back to Arimo, and a real PDF with Arimo embedded. Two smaller things fall out. `begin_pdf()` normalises a filtered `fontDir` to an array, because the package layers append to it and the public `gfpdf_mpdf_class_config` filter has long been handed a bare string — harmless while nothing appended, fatal now. And font aliases are asserted on the package rather than on mPDF's merged config: `initFontRegistry()` deduplicates that map by value, so `arial` and `helvetica` both naming Arimo collapse to one. Fixed on the fork in GravityPDF/mpdf#36; this tightens once the plugin re-pins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
initFontRegistry()ends by deduplicating each of the six properties it merged from the font packages. That is right for five of them —backupSubsFont,BMPonly,sans_fonts,serif_fontsandmono_fontsare lists of font keys, where a repeat is noise.fonttransis the sixth, and it is not a list. It maps an alias to the font it resolves to, keyed by alias, andarray_unique()compares values. So two aliases naming one font collapse to whichever came first — silently, with no warning and nothing in the merged config to hint at it.A package registering the obvious pair for a metric-compatible substitute:
ends up with
arialalone, andfont-family: helveticastops resolving for a reason the package author cannot see from their own code. Aliasing several names onto one font is the ordinary case for a substitute family, not an edge one.Try it
Register a package whose
getFontAliases()returns two aliases for the same font, constructMpdf, and inspect$mpdf->fonttrans. Before this change it holds one entry; after, both.Test plan
InitFontRegistryTest::testEveryAliasSurvivesEvenWhenTheyNameOneFontcovers it, withTestFontRegistrationAgaining the two-aliases-one-font pair. Reverting the fix errors the test withUndefined array key "aliasTwo", so it pins the behaviour rather than merely describing it.Full suite 1,123 tests / 2,644 assertions, OK;
composer csclean.The change
The precedence the original expressed is unchanged — package values still come before whatever is already on the property, and for
fonttransarray_merge()'s own last-wins-per-key behaviour is the correct rule for a map. Only the deduplication is skipped, and only for the one property where "duplicate" was being measured against the wrong thing.