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

PHP 8.0+ version #316

Merged
merged 11 commits into from
Feb 27, 2023
Merged

PHP 8.0+ version #316

merged 11 commits into from
Feb 27, 2023

Conversation

bnomei
Copy link
Contributor

@bnomei bnomei commented Feb 20, 2023

this PR

  • drops PHP 5.6 and 7.4 support
  • upgrades the code to PHP 8.0 (done automatically via rectorphp)
  • formats code psr-12 (via laravel pint)
  • closes AVIF and color-extractor issues
  • adds example files for every image format, but excludes them from release zips via .gitattributes file
  • and adds minor changes to readme

@claviska
Copy link
Owner

Thanks for bringing the library up to speed with current PHP versions! I'm perfectly fine accepting this PR, but it's going to require a version bump to 4.0 to prevent breaking things for existing users.

Before we proceed, are there any other breaking changes you think should be made?

@bnomei
Copy link
Contributor Author

bnomei commented Feb 22, 2023

like you said in #284 i would also v4 mainly to upgrade php version. the api in its current state does not need changing imho - it get the job done well.

@bnomei
Copy link
Contributor Author

bnomei commented Feb 22, 2023

EXIF
personally i would love to see the exif #157 issue solved but that would also mean to introduce an option for people that depend on the data being removed to keep it that way. maybe copying exif also has and performance impact (when a bulk of images is created via a CMS etc)

@bnomei
Copy link
Contributor Author

bnomei commented Feb 22, 2023

FORMAT SPECIFIC OPTIONS
some GD image methods would accept additional options that use defaults but can not be set right now. maybe all toXXX() api methods could get a new generic $option value that gets expanded as params into the functions if it exists?

example:

    public function toString($mimeType = null, $quality = 100, $options = null)
    {
        return $this->generate($mimeType, $quality, $options)['data'];
    }

    protected function generate($mimeType = null, $quality = 100, $options = null)
    {
        // OTHER CODE
        is_array($options) ? 
           imageavif($this->image, null, $quality, ...$options) :
           imageavif($this->image, null, $quality);
    }

used like this...

   echo $image->toScreen('image/avif', 80, ['speed' => 8]);

@claviska
Copy link
Owner

maybe all toXXX() api methods could get a new generic $option value that gets expanded as params into the functions if it exists?

Should we also combine $quality into $options since it varies with some formats? Do you want to take this on, or would you rather me do it?

@bnomei
Copy link
Contributor Author

bnomei commented Feb 23, 2023

i will add a suggestion to my PR during the next few days.

@bnomei
Copy link
Contributor Author

bnomei commented Feb 24, 2023

@claviska updated the PR with toXXX() adjustments and added type declarations.

decided that i will not work on the exif issue. feel free to release v4 without it.

$width = $maxHeight * $this->getAspectRatio();
} else {
$width = $maxWidth;
$height = $maxWidth / $this->getAspectRatio();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we need to add round() to this to prevent the following error:

PHP Deprecated: Implicit conversion from float 375.234521575985 to int loses precision in /Users/claviska/SimpleImage/src/claviska/SimpleImage.php on line 958

I noticed this when using bestFit() in the example, as it was causing the image to not render. This might affect other methods in a similar way.

To replicate:

cd SimpleImage/example
php -S localhost:8080

Then a open a browser at http://localhost:8080/index.php and notice the broken image.

// Reduce to max width
if ($width > $maxWidth) {
$width = $maxWidth;
$height = $width / $this->getAspectRatio();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to use round() or we can accept a float in resize() and convert it there. Your call.

@claviska
Copy link
Owner

I just wanted to say that this PR looks really good. I only noticed that one issue with bestFit() passing a float to resize(), breaking the image. Again, the same problem might be affecting other methods so we may want to check for that.

It's been awhile, but IIRC most of the GD functions require integers so we should probably use round() whenever we divide. Do you want to take a stab at getting those updated as well?

@bnomei
Copy link
Contributor Author

bnomei commented Feb 25, 2023

sure. i can do that.

@bnomei
Copy link
Contributor Author

bnomei commented Feb 25, 2023

there are lots remaining related resource/GDImage/false and since some methods return false instead of throwing an error. but I think it should be good enough for now.

@claviska claviska merged commit 1d2293c into claviska:master Feb 27, 2023
@claviska
Copy link
Owner

Thanks so much for this. A 4.0.0 tag has been created.

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.

2 participants