claviska / SimpleImage

A PHP class that makes working with images and GD as simple as possible.
MIT License
1.38k stars 382 forks source link

Optional SSL validation #300

Closed VictorWesterlund closed 2 years ago

VictorWesterlund commented 2 years ago

Added a new optional $sslVerify argument to the SimpleImage constructor and fromFile method which, when true, disables the OpenSSL certificate peer validity checks.

$img = new SimpleImage("https://host.local/img.png", false);
// .. and also
$this->fromFile("https://host.local/img.png", false);

This is useful for self-signed certificates or other cases when SSL validity checks are explicitly unwanted.


I have mixed feelings about using a valuable argument on the constructor for something that has this limited real world use. I can't think of many situations where skipping SSL validation would be good practice outside testing - so perhaps that can be improved.

claviska commented 2 years ago

Would it be better to do this with a method that sets as a flag on the instance? e.g.

$image
  ->sslVerify(false) // sets an internal flag that all function reference an internal flag
  ->fromFile('image.jpg')
  ->flip('x')
  ->toScreen(); 

It seems like it would be easier to use this way and keeps the argument list for each function simpler.

VictorWesterlund commented 2 years ago

sets as a flag on the instance

That's a really good idea!

Unfortunately, it would be a breaking change. fromFile() is called from the constructor of the class and would have to be refactored so it doesn't run before the flag kicks in. https://github.com/claviska/SimpleImage/blob/01b9130c2755270ec9a241360728e3cab1e68b4c/src/claviska/SimpleImage.php#L61-L66

claviska commented 2 years ago

Another idea is for the constructor to accept a second argument which sets various options (only sslVerify for now) and instead of an sslVerify() method we could add a setOption($name, $value) method to allow configuration during both init and when chaining.

This expands the scope of your initial PR, but it will establish a pattern for internal flags that will be useful for similar settings. What do you think?

VictorWesterlund commented 2 years ago

Yeah that's even better! I can have a crack at it ASAP; and create a new PR for that (out of scope for this one as you mentioned)

claviska commented 2 years ago

Added in #301