WIP Make GD Extension Optional

[oleibman]

Fix #3719. Also #367, #469, #1384. It clearly comes up a lot. It has been rejected every time, and maybe it should be rejected now as well. For now, it can't hurt to submit a draft PR and gather feedback. Latest issue makes a decent case for this change, but will it open us up to a series of issues where user fails because GD is not enabled?

Based on approximately 30-35 unit tests that fail/error when run without GD, I believe the following is an exhaustive list of what is not supported (because they use functions available only when GD is enabled). Note that function getimagesize is available even if GD is not enabled.

• reading an image in an Xls file. This is the most likely source of problems if GD is not enabled.
• cloning a MemoryDrawing.
• creating a MemoryDrawing from string.
• setting imageResource for MemoryDrawing.
• rendering a chart.
• Writer/Xls support for GIF/BMP drawings (Xls requires that these be converted to PNG).
• Writer/Xlsx support for GIF/BMP drawings as background images for comments. We convert these to PNG. I am not convinced that these conversions are necessary, and may investigate further if this PR is implemented.
• Dompdf and Tcpdf may require GD under certain circumstances.
• exact font measurements.

I have changed the code so that any attempt to use one of the unsupported functions identified above (except exact font and Dompdf/Tcpdf) will throw an exception ('Required PHP extension GD is not enabled') if they are attempted when GD is not enabled; this will, I hope, make it clear to the end user what the problem is. Dompdf/Tcpdf already generate a similar message when GD is needed but unavailable. For exact fonts, we will just fall back to approximate, which we sometimes do even when GD is available.

I will leave this PR in draft status for a while to see if there is feedback pro or con before attempting to merge it.

This is:

• a bugfix
• a new feature
• refactoring
• additional unit tests
• response to multiple requests

Checklist:

• Changes are covered by unit tests
  • Changes are covered by existing unit tests
  • New unit tests have been added
• Code style is respected
• Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
• CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
• Documentation is updated as necessary

Why this change is needed?

Provide an explanation of why this change is needed, with links to any Issues (if appropriate).
If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.

[PowerKiKi]

My opinion did not change and I am still against this.

My main concern is that your app might consumes files on which you have no control. Typically a user that upload a file that you are supposed to read in some ways. Your code might seems to work in 99% of cases, until a user submit a file that contains an image. Then the whole process crash entirely. From my point of view such unpredictability is an unacceptable DX.

We either require GD or we don't. We cannot mostly-do-not-require-GD-but-sometimes-we-still-requires-GD-anyway.

The four issues you mention spread over the last 6 years and gather about 10-ish users. That does not seem worth the effort IMHO.

Also the proper solution already exists and is described here: #1384 (comment)

We should not jeopardize the stability of all users for the few users that are willing (and technically able to) accept the risks that come with that.

[ruudk]

My main concern is that your app might consumes files on which you have no control. Typically a user that upload a file that you are supposed to read in some ways. Your code might seems to work in 99% of cases, until a user submit a file that contains an image. Then the whole process crash entirely. From my point of view such unpredictability is an unacceptable DX.

If your app reads unknown files, then yes, you should make sure the gd extension is installed. If you use this library to read a few simple spreadsheets, then no.

If you use this library to write some files, it's different. You know what you are doing to write these files. So when you don't draw anything that requires gd, then you don't need it.

[PowerKiKi]

You know what you are doing to write these files

If that is your case, then feel free to tell composer that you know what you are doing once and for all.

But I'd rather have knowledgeable developers conscientiously opt-in to potentially shoot themselves in the foot, rather that shoot in the feet of the majority of not-so-involved developers that either don't have the time or skill to figure this out.

One solution not mentioned here (but mentioned in other threads), that may satisfy all of us is to split PhpSpreadsheet into multiple packages, one for each reader and each writers. In that case you could install only, say, phpoffice/phpspreadsheet-writer-xlsx and not be required to have GD installed. ... ... but even then, I am not sure this would be a viable solution because cloning a spreadsheet (which happens in the "core", not in readers/writers) does require GD 🤷 ... ... But that's, IMHO, way too much work for a solution that might not even work for the case of GD, and all of that for a minority of users...

[ruudk]

You know what you are doing to write these files

If that is your case, then feel free to tell composer that you know what you are doing once and for all.

I think you misunderstood what I meant with "know what you are doing". I meant: when you create a simple spreadsheet, you don't need GD. If you use anything that requires it, it will show up on runtime, while developing / testing it. It will tell you: "you need gd". That's perfectly fine, right?

The link you mention is not related, as my point is that I want to get rid of gd extension on production. So telling Composer that this is installed, while it isn't, is even a bigger foot gun.

[PowerKiKi]

show up on runtime, while developing / testing it

Maybe, maybe not. Depends entirely on how you work and how well tested your code is. And PhpSpreadsheet is a popular tool also amongst less experienced developers. You cannot expect of them to follow the latest best practice at all times (unfortunately).

I want to get rid of gd extension on production

Why is that so important to you ? What is your use-case ?

telling Composer that this is installed, while it isn't, is even a bigger foot gun.

And yet I don't see any difference between this PR and lying to composer that GD exists. In both cases you saying "my code may or may not crash, I take the entire responsibility for it, please don't help me". If you have processes in place that can guarantee that your code will work in all cases, then go ahead and configure composer to stop bothering you.

I must say I don't understand people who are fighting so hard against the very reason why composer exists. composer was created to ensure that all your dependencies exists (and are automatically installed when possible). And to do that at "build" time, not at runtime. And it has been exceptionally beneficial to the entire PHP ecosystem. Why fight against it here ? Especially when you can disable the specific part that bothers you ?

[ruudk]

I'm going to leave this discussion. I don't fight composer at all. What makes you believe that I do? I love it.

I don't want gd extension in production to minimize our Docker images and to remove yet another possible attack vector.

[PowerKiKi]

I'll go ahead and close this, because I am still not convinced that there exists a strong enough use-case that cannot be solved today via composer configuration but could be solved in a robust way via our own code in a reasonable fashion (meaning without re-implementing composer features ourselves).

[oleibman]

@PowerKiKi That's fine, I just wanted to demonstrate a POC and follow any discussion concerning it. I find compelling the fact that there is an easy command line option to ignore specific requirement(s), so I was planning to close this anyhow.
On another note, is it time for a new release?

[PowerKiKi]

is it time for a new release?

I'd like to review the mixed types if we can specify them better (like #3860), since it would be a breaking change to do that later. And then release. So maybe a few days... depending on the time I can spend on that... any help is welcome as usual ;-)