-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow the Fonts to be loaded via Packages #841
base: development
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jakejackson1 and thank you for this PR
Left you some comments while reading through the code.
} else { | ||
$config[$id] = array_merge($val, $defaults[$id]); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array_merge()
should be avoided in loops (and we're in two loops)
would it be possible to gather data in the loop instead, and perform a single array_merge at the end ? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if that'll be possible. Would love it if you could take a closer look and brainstorm some ideas.
I generally like this idea, just a few remarks for now:
|
@finwe I'm not really sure about your first bullet point, as we're talking about 19M (it just took me 26 seconds to clone Jake's repo). If the end user doesn't need them, then it's a pure waste. Could we instead update the installation guide with heads-up, and use Composer's suggest feature to hint the user about available packages instead ? |
Will give it some more thought. |
I've rebased this PR on the latest development branch and tidied up a couple things based on @machour feedback (thanks).
|
Hi! Is this still relevant? We're using Docker and right now mpdf is by far the fattest package due to the fonts directory. Composer's |
Yes, it is relevant. Yes, composer suggest for the most common font packages will be in place after the mechanism is finalized and fool-proofed. |
This is the first attempt at decoupling the fonts from Mpdf. The code / fonts pulled out from this PR can be found here: https://github.com/jakejackson1/mpdf-unicode. In the separate package is the fonts, a font configuration file, and a language to font configuration file.
Before I spent too much time on it, I wanted to open this up to discussion about if this is the right direction to take. I envision we split up the current fonts package into a number of smaller modules that cater to the different script types: unicode (name chosen because of the vast array of characters Dejavu and Freesans supports), CJK, Arabic, Egyptian ect. We can then add each as a suggested composer package. The benefit to this approach (besides reducing the size of core) is it'll allow users interested in each language to maintain fonts suitable for their language (Bengali comes to mind) without effecting the core.
For those that would like to test this out, the only change required is to use the new
fontRegistry
configuration key when you initialise mPDF:I thought about loading known packages automatically, but for now opted for manual initialisation. We can certainly explore the automated option if enough people like the idea.
From a technical perspective, this PR does not negatively effect any of the existing configuration keys to do with fonts and it all works together seamlessly (or that's the idea anyway - no tests yet).