dompdf icon indicating copy to clipboard operation
dompdf copied to clipboard

chmod fonts folder as part of composer post install.

Open chopfitzroy opened this issue 9 years ago • 7 comments

I did a quick search through current and past issues/pulls and could not find anything so please forgive me if this has been brought up before.

I just came across an issue where I spent about 3 hours trying to figure out why my fonts were not working on a fresh install of a project I am working on. I then realised I had not updated the permissions on:

vendor/dompdf/dompdf/lib/fonts

Now I am not 100% sure if I should need to update the permissions on these folders or if there is just something funky going on with my environment. However either way I am sure more people than just me have run into this issue, so I was thinking as part of the composer install we could set the permissions on this folder and that way if it was already right no harm done and if not we could save people hours of head ache :)

On my own project I have just added this to the composer.json:

"scripts": {
    "post-root-package-install": [
      "chmod -R 777 vendor/dompdf/dompdf/lib/fonts"
    ],
    "post-install-cmd": [
      "chmod -R 777 vendor/dompdf/dompdf/lib/fonts"
    ],
    "post-update-cmd": [
      "chmod -R 777 vendor/dompdf/dompdf/lib/fonts"
    ]
}

Before I submit a PR can anybody see any reason why this would be a bad idea? Obviously we don'y have to set the permissions to 777 this was just a quick fix.

Cheers.

chopfitzroy avatar Jul 13 '16 22:07 chopfitzroy

Hm. Not sure this is the best solution to the issue. Ideally the user will specify their font directory and font cache directory. I think moving forward it would be good to keep the dompdf space clean (especially if the install is via composer). I'm not sure, though, what would be the best solution. We could make these directories point to the temp directory, but that seems a bit hacky.

Maybe displaying a message post-install about the need to specify these directories when using dompdf?

bsweeney avatar Jul 13 '16 22:07 bsweeney

Hey @bsweeney I like the idea of a post install message, and I agree temp directories feels hacky at best.

Could we potentially offer some sort of post install set up script similar to laravel's artisan that will ask the user to specify fonts directories (among other things) and potentially offer a fall back if unspecified?

This does however add another layer of complexity to the install process and I am not sure how much you would like to streamline this process.

chopfitzroy avatar Jul 13 '16 23:07 chopfitzroy

I'm not so much worried about the install process as what it would entail to add this kind of functionality. The settings are currently hard-coded into the Options class. That was done, partially, to make it feasible to remove the dompdf_config.inc.php file and the constants defined by it.

What are you thoughts as to where these settings would be stored? A file that the Options class reference during instantiation? Modifying the class file directly? The former certainly makes more sense. If the file exists we could throw a warning about the missing settings and that defaults will be used.

I would actually consider using the temp directory as the default if we have an install script and run-time warning. The main problem from using the temp directory would be that the user would experience a bit of a performance hit from having to parse the font files every time.

bsweeney avatar Jul 14 '16 01:07 bsweeney

I can't think of any reason as to why this would not work, my PHP knowledge is somewhat lacking is there any drawbacks to modifying the class file directly? And I am correct in understanding if a setting was missing we would use the temp directory as a fallback? I think that is reasonable.

chopfitzroy avatar Jul 14 '16 01:07 chopfitzroy

I wouldn't want to mess with the class file in order to avoid any potential issues with the modification. Using an external file to store local defaults should be fine, and is how we're handling the bundled fonts (see dompdf_font_family_cache.dist.php).

I think making the default in the class use the temporary directory makes sense.

bsweeney avatar Jul 14 '16 14:07 bsweeney

Awesome sounds good to me, is there anything I can do to help???

chopfitzroy avatar Jul 14 '16 21:07 chopfitzroy

You're welcome to hack at it if you want. Just submit a PR when you're done and I'll review and make any changes I think might improve it.

bsweeney avatar Jul 14 '16 21:07 bsweeney