Closed Art4 closed 6 months ago
I've installed intervention/image v3 and made the roughest adjustments to get an overview of the further work required. Looking at the tests there seems to be some changes required with the encoding, because this modifier has to be run at the last step.
Brief interim report
I've replaced rectangle() with drawRectangle() and found some differences using border between v2 and v3.
Note: I've created a simple comparison tool to check the results for Glide v2 and v3 with the same query. See https://gist.github.com/Art4/95c476dc8aa42e8668db792c03e103e1
If I add fm=png
to force a format change the border colors will be correct. I assume that the base for a new image will be filled with color #fff instead of #000.
So, this is the current situation:
I will finish these points and fix the tests and psalm errors to finalize this first PR.
Todo for separate PRs:
Hey @Art4 thanks for using my PR and making all those changes.
The only thing I'm concerned is about sizing. Glide v2 relied a lot of Intervention aspectRatio
method to keep images resizing properly https://image.intervention.io/v2/api/resize
Since this useful helper was removed from the official intervention v3, I wonder what we could do for this refactor to make sure it will behave as the previous version. Specially w/ fit
parameter https://glide.thephpleague.com/1.0/api/size/#fit-fit
Since this useful helper was removed from the official intervention v3, I wonder what we could do for this refactor to make sure it will behave as the previous version. Specially w/ fit parameter https://glide.thephpleague.com/1.0/api/size/#fit-fit
We could also consider changing Glide's API/options as per changes to Image, if trying to retain existing options is too problematic.
For e.g. I quite like the new "cover", "contain" naming in Image as they corelate to similar behavior in CSS.
I'm nearly done with fixing all tests, but are struggling with Mockery on this last test. See https://github.com/Art4/glide/actions/runs/7656671526/job/20865541487
Error: 1) League\Glide\Manipulators\BorderTest::testRunOverlay Mockery\Exception\InvalidCountException: Method width(<Any Arguments>) from Mockery_1_IteratorAggregate_Intervention_Image_Interfaces_ImageInterface should be called exactly 1 times but called 0 times.
Could someone help me why the provided closure is not called?
@Art4 Since the drawRectangle()
which uses the closure is mocked the closure would never actually be used/run.
@Art4 I fixed the test and pushed the changes to your branch.
@Art4 I just have one objection with the current strategy to encode the image.
It seems the Encode Manipulator was commented out from ServerFactory::getManipulators()
and moved to Api::run
. I can see the reason why it was made.
But I still think that the Encode Manipulator is still vital for this and my suggestion is to keep that, unless @ADmad has a different opinion.
I managed to fix the Encoded Manipulator on my branch and I was able to test it on production properly after the changes I did here https://github.com/Art4/glide/pull/2. Would you mind to check it again?
@konnng-dev Yes we should keep the encoder manipulator since you have managed to fix it.
It seems the Encode Manipulator was commented out from
ServerFactory::getManipulators()
and moved toApi::run
. I can see the reason why it was made.
Good catch. I've uncommented this, so Encode Manipulator is used again.
But I still think that the Encode Manipulator is still vital for this and my suggestion is to keep that, unless @ADmad has a different opinion.
I managed to fix the Encoded Manipulator on my branch and I was able to test it on production properly after the changes I did here Art4#2. Would you mind to check it again?
I would recommend to create a separate PR in this main repo about this issue, instead in my private repo, to keep the version history and discussions in this repo.
I still have to do a more thorough review of the PR but I am going to merge it so that @konnng-dev can submit his changes on top.
Thanks guys for all your work. Please start trying out the 3.x branch as due to the nature of this lib passing tests are no guarantee that the images generated are what we expect 😄.
This PR will drop support for PHP 7.4 and 8.0, but will allow installation of intervention/image v3. Because there will be breaking changes I propose this for Glide v3.
Refs #288.
Closes #377, #378, closes #379.