Skip to content
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

Open
wants to merge 3 commits into
base: development
Choose a base branch
from
Open

Allow the Fonts to be loaded via Packages #841

wants to merge 3 commits into from

Conversation

jakejackson1
Copy link
Contributor

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:

$mpdf = new Mpdf\Mpdf([
	'fontRegistry' => new \Mpdf\Fonts\FontRegistry([
		new \Mpdf\Fonts\Unicode\Config\FontVariables(),
	]),
]);

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).

@finwe finwe added this to the 8.0 milestone Nov 3, 2018
@finwe finwe mentioned this pull request Nov 30, 2018
@finwe finwe modified the milestones: 8.0, 9.0 Mar 6, 2019
Copy link
Contributor

@machour machour left a 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.

src/Language/LanguageToFontRegistry.php Outdated Show resolved Hide resolved
src/Language/LanguageToFontRegistry.php Outdated Show resolved Hide resolved
} else {
$config[$id] = array_merge($val, $defaults[$id]);
}
}
Copy link
Contributor

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 ? 🤔

Copy link
Contributor Author

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.

src/Language/LanguageToFontRegistry.php Outdated Show resolved Hide resolved
src/Fonts/FontRegistry.php Outdated Show resolved Hide resolved
@finwe
Copy link
Member

finwe commented Mar 13, 2019

I generally like this idea, just a few remarks for now:

  • I'd very much like to keep at least Dejavu along with the main package - UTF support out of the box is one of the main mPDF selling points. Sure, these fonts can be split, but then the package should be a root dependency of the main package
  • The configuration is hands down clean and extendable, yet it seems a bit too complex for a newcomer - could we come up with a simpler way of registering a new font than to create multiple classes and linking them together?
  • How about some "font autodiscovery"? So that only requiring a composer package is needed. That would simplify at least including of already existing font packages.

@machour
Copy link
Contributor

machour commented Mar 13, 2019

@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 ?

@finwe
Copy link
Member

finwe commented Mar 13, 2019

Will give it some more thought.

@jakejackson1
Copy link
Contributor Author

I've rebased this PR on the latest development branch and tidied up a couple things based on @machour feedback (thanks).

@finwe

  • I'm with @machour on this, let's strip all the fonts out of core and allow users to pick and choose what font packages they want (or just use the Core mPDF fonts)

  • The auto-discovery feature in Aura looks better than the Laravel version. I'll have a tinker when I can and look at implementing it.

@acasademont
Copy link

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 suggest would be the best way to proceed imho!

@finwe
Copy link
Member

finwe commented Feb 11, 2020

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.

@Klap-in Klap-in mentioned this pull request Aug 4, 2021
@finwe finwe modified the milestones: 10.0, 9.0 Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants