Try to create config directories if non existant #3115
Conversation
The patch will allow dompdf to try creating tmpDir, fontDir and fontCache if they do not exist on the filesystem - and will trigger a warning if they can't be created. Even if the Options.php file mention that pathes should exist, a number of people will set the options and won't even see that the folders weren't created. Trying to create them by default will : - Avoid bugs / lack of performances when directories are created - Will warn the user that his configurations are not suitable if necessary. Later, it might also be possible to change the trigger_error to an exception, but right now, it would be a BC break.
|
This seems reasonable. |
There was a problem hiding this comment.
Thinking on this again, I wonder if we just require that the user create the necessary directories prior to setting them in Dompdf. The main reason being that if we really want to do this we should probably allow the user to specify the permissions, which complicates things a bit. We could, instead of making the directory, just throw an exception if the specified directory is invalid so that the user knows they forgot to do something.
I'm still open to this change, just trying to think through potential unintended consequences.
| @@ -848,6 +848,9 @@ public function getDpi() | |||
| */ | |||
| public function setFontCache($fontCache) | |||
| { | |||
| if (!is_dir($fontCache) && !mkdir($fontCache)) { | |||
There was a problem hiding this comment.
Should we make the directory recursively (e.g., mkdir($dir, 0777, true))?
There was a problem hiding this comment.
Yep, that would be definitely better, even if the 777 permission might be problematic somehow
There was a problem hiding this comment.
Noting for reference: I used 0777 since it's the default when no value is provided. I agree it's not ideal, though probably the best option so user can remove directories created by their web server.
Yep, I agree - however, I didn't want to have a BC break by preventing people to run if the folder doesn't exist. Might it be a simple warning message for the next version, and a break on the next one? |
| @@ -848,6 +848,9 @@ public function getDpi() | |||
| */ | |||
| public function setFontCache($fontCache) | |||
| { | |||
| if (!is_dir($fontCache) && !mkdir($fontCache)) { | |||
| trigger_error("Unable to create fontCache - a valid value should be specified.", E_USER_WARNING); | |||
There was a problem hiding this comment.
Thinking Dompdf should return without setting the option if it's unable to create the directory.
There was a problem hiding this comment.
I'd say that it will probably go unnoticed and generate some unexpected behaviors if Dompdf is doing so (slowness, refetching fonts in a loop for nothing). Mostly, if those dir are set but aren't existing and we can't create them, there's probably an issue to fix worth preventing to load it.
However, this is a BC break - so it might be worth to put a warning for users for a few versions, then do something stricter on the next major version, for example?
There was a problem hiding this comment.
Sometimes it's hard to balance between just making it work and asking developers to do their job 😅. You're probably right that a lot of users will miss the warnings. Is your suggestion to not make the directory but add the warning, then in a future major release add the mkdir functionality?
The patch will allow dompdf to try creating tmpDir, fontDir and fontCache if they do not exist on the filesystem - and will trigger a warning if they can't be created.
Even if the Options.php file mention that pathes should exist, a number of people will set the options and won't even see that the folders weren't created.
Trying to create them by default will :
Later, it might also be possible to change the trigger_error to an exception, but right now, it would be a BC break.