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

Encode using media type instead of extension #410

Closed
wants to merge 26 commits into from

Conversation

deluxetom
Copy link

If an image had image/x-webp or another format supported by Intervention but not defined in supportedFormats, it would fail.

This uses encodeByMediaType instead which uses the proper values and available formats defined by Intervention

src/Api/Encoder.php Show resolved Hide resolved
src/Api/Encoder.php Outdated Show resolved Hide resolved
tests/Api/EncoderTest.php Show resolved Hide resolved
@@ -58,17 +58,11 @@ public function getImageManager(): ImageManager
/**
* Set the manipulators.
*
* @param ManipulatorInterface[] $manipulators Collection of manipulators.
* @param array $manipulators Collection of manipulators.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
*/
* @throws \InvalidArgumentException
*/

Copy link
Author

Choose a reason for hiding this comment

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

@ADmad good catch, I added the missing phpdoc

src/Api/Api.php Outdated
* @param ImageManager $imageManager Intervention image manager.
* @param array $manipulators Collection of manipulators.
* @param ImageManager $imageManager Intervention image manager.
* @param list<ManipulatorInterface|null> $manipulators Collection of manipulators.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think |null is necessary

Copy link
Author

Choose a reason for hiding this comment

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

agreed, its been fixed

@ADmad
Copy link
Collaborator

ADmad commented Dec 17, 2024

Something went wrong, testsuite is failing.

@deluxetom
Copy link
Author

Something went wrong, testsuite is failing.

GitHub is having some issues, I can't restart the workflow, tests are all green locally:

> ./vendor/bin/phpunit
PHPUnit 11.5.1 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.4.1
Configuration: phpunit.xml.dist

...............................................................  63 / 216 ( 29%)
............................................................... 126 / 216 ( 58%)
............................................................... 189 / 216 ( 87%)
...........................                                     216 / 216 (100%)

Time: 00:00.327, Memory: 16.00 MB

OK (216 tests, 455 assertions)

I'll try to fix as soon as I can

@deluxetom
Copy link
Author

@ADmad it seems heic isn't supported anymore, I'm not sure I understand why, it started when I merged the 3.x branch from upstream.

@ADmad
Copy link
Collaborator

ADmad commented Dec 17, 2024

Aha, I remember now, heic decoding is broken on ubuntu-24.04. I forgot about that and updated the github actions config to use 24.04 instead of 22.04 few hours ago. Please revert the image for tests job to ubuntu-22.04 and the error will be gone.

@deluxetom
Copy link
Author

@ADmad all green now, let me know if there's anything you want me to update

@ADmad
Copy link
Collaborator

ADmad commented Dec 17, 2024

Looks good to go. I'll just have a look again with fresh eyes tomorrow before merging.

@ADmad
Copy link
Collaborator

ADmad commented Dec 19, 2024

@deluxetom I re-thought about the problem your PR aims to address and come up with an alternative which require much lesser code changes #415

@ADmad
Copy link
Collaborator

ADmad commented Dec 19, 2024

Closing as the alternative PR is merged. Thank you @deluxetom.

@ADmad ADmad closed this Dec 19, 2024
@deluxetom deluxetom deleted the 3.x branch December 19, 2024 19:21
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.

3 participants