Add Symfony Image Component #21820

avalanche123 · 2017-03-01T17:09:39Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT
Doc PR	See https://github.com/romainneutron/symfony-docs/tree/symfony-image-component

This component is a port of Bulat Shakirzyanov's Imagine library (https://github.com/avalanche123/Imagine) into Symfony.

romainneutron · 2017-03-01T17:12:17Z

I just updated the description

stof · 2017-03-01T17:16:00Z

I'm not a fan of integrating it in Symfony as is. The Symfony BC policy would forbid adding any new feature in the component after the initial release, unless we mark all interfaces as internal, due to the current architecture. And marking them as internal would also remove the BC promise for consuming code.

fabpot · 2017-03-01T17:28:23Z

@stof The thing is that the original library has enough people to maintain it. I would not like to see it unmaintained. It's been stable for years, why would we want to change the internal behavior? I mean, we can do that in the coming months/years, benefiting from our processes and the power of the Symfony community. This is just a first step.

Thanks @avalanche123 for making that possible.

avalanche123 · 2017-03-01T17:29:48Z

@fabpot you're welcome, happy to see Imagine finding a new home, thank you!

nicolas-grekas · 2017-03-01T17:34:51Z

I'm supporting this proposal. Two questions to start:

should we make the component experimental on 3.3 so that we can change it while working on 3.4? (I'd do it with a note in the README + CHANGELOG, not in the code, too cumbersome, would you?)
should we ~~move the image fixtures to git LFS to~~ not make the SF git history grow too much? (maybe a bad idea, see https://medium.com/@megastep/github-s-large-file-storage-is-no-panacea-for-open-source-quite-the-opposite-12c0e16a9a91)

fabpot · 2017-03-01T17:37:57Z

Marking the whole component as experimental for 3.3 looks like a good compromise to me.

stof · 2017-03-01T18:14:31Z

should we move the image fixtures to git LFS to not make the SF git history grow too much? (maybe a bad idea)

I don't think we will change these fixtures often, meaning we will almost never store a new blob for them.
However, we should exclude them from the archive download, to avoid having every Symfony user downloading them for nothing when using composer.

ausi · 2017-03-01T19:39:32Z

Any interest in merging contao/imagine-svg too?
I’m one of its maintainers.

Seldaek · 2017-03-01T20:27:59Z

While I'm not convinced moving standalone libs into Symfony is a good trend, I doubt I'll convince you not to do it, so I'd like to insist on one aspect which I think is important in terms of code design and quality: Please keep in mind it should work with very large images and that nothing should load the whole image in memory and do copies and so on. All gd/imagick transforms can be applied on files directly if done properly IIRC, and I'd like to avoid a repeat of php-imagine/Imagine#479 if possible :) Maybe generating large fixture files while running the test suite would be an idea, or running those tests with a very low memory limit to make sure any mistake is caught early?

andrerom · 2017-03-02T07:37:04Z

Would also like to see the above from @Seldaek being solved before this is stable (or even merged), a customer had this exact issue on a certain .sh platform, so I guess SensioCloud would have same issue @fabpot.

fbourigault · 2017-03-02T08:10:17Z

As Git LFS is not suitable for open-source projects, you can host fixtures on some CDN and have a phpunit listener that will download those fixtures on-demand with a sha256 checksum check. This solution allows to:

save space in the main git repository.
allow use of very large images in tests.
allows you to enable caching on travis-ci.
allows to run tests against a large set of images.
is probably future-proof depending on which features would be added to the Image component.

I already used a such setup for a photo application with 500MB of pictures without any issue.

javiereguiluz · 2017-03-02T08:28:22Z

Please, let's keep things simple. Git LFS and CDN would only complicate things. The "problem" is quite limited and (probably) easy to solve:

Tests/ is just 3,617,110 bytes
Resources/ is 9,241,014 bytes ... and you can't move this to Git LFS or a CDN

Our biggest problems are:

Resources\Adobe\CMYK\ files are massive! Do we need all of them?
Resources\Adobe\Profile Information.pdf is 444 KB. Do we need it?
Tests\Fixtures\cat.gif is 1 MB. Please, create a different GIF file.
Tests\Fixtures\pixel-CMYK.jpg is 744 KB. Can we replace it with a smaller image?
Tests\Fixtures\font\Arial.ttf is 773 KB. Can we replace it with a smaller and free font?

nicolas-grekas · 2017-03-02T10:50:55Z

@ausi that could be possible, but that'd require a license and copyright change. Would you agree on that? Anyway, I suggest to wait for this PR to be accepted and merged, then you could make a PR to add contao/imagine-svg if you'd agree to.

ausi · 2017-03-02T11:05:42Z

@nicolas-grekas changing the copyright and license should be no problem.
OK, then I wait until this PR gets merged.

sstok · 2017-03-02T11:29:11Z

Can't we use the same approach like the old ICU component? A separate repository for the data, you already have Git so that shouldn't be a problem.

romainneutron · 2017-03-27T19:53:17Z

Hi everybody,

I think we're now ready, and I'm calling for reviews :)
I've addressed the issues you pointed

moved the large fixtures to a dedicated repo
removed unnecessary ICC profiles
fixed the test suite (it should be alright now, no Image tests are failing)
I've pushed a doc branch mentioned in the PR header that contain a commit of the Imagine documentation adapted to Symfony docs. It definitely needs some work, but I'm pretty sure it would help any doc writer.
The whole component is marked as EXPERIMENTAL

I may have forgot something. Looking forward to read your comments.

stof · 2017-03-28T07:39:32Z

.github/imagick.sh

+
+if [ ! -e ./ImageMagick-$IMAGEMAGICK_VERSION ]
+then
+    wget http://www.imagemagick.org/download/releases/ImageMagick-$IMAGEMAGICK_VERSION.tar.xz


I would use https (they support it)

stof · 2017-03-28T07:46:36Z

src/Symfony/Component/Image/Gd/Image.php

+        $save = 'image'.$format;
+        $args = array(&$this->resource, $filename);
+
+        // Preserve BC until version 1.0


IMO, we don't need to keep the BC layers of Imagine 0.x here

stof · 2017-03-28T07:47:49Z

src/Symfony/Component/Image/Gd/Image.php

+        $this->setExceptionHandler();
+
+        if (false === call_user_func_array($save, $args)) {
+            throw new RuntimeException('Save operation failed');


the error handler must be restored before this exception. I suggest using try/finally

Please check the whole component for similar leaks of the internal error handler in case of exceptions

stof · 2017-03-28T07:52:13Z

src/Symfony/Component/Image/Gd/Image.php

+        return in_array($format, $formats);
+    }
+
+    private function setExceptionHandler()


should be renamed, as it is setting the error handler, not the exception handler

stof · 2017-03-28T07:52:33Z

src/Symfony/Component/Image/Gd/Image.php

+        }, E_WARNING | E_NOTICE);
+    }
+
+    private function resetExceptionHandler()


do we really need a private method here (badly named) ?

stof · 2017-03-28T08:24:15Z

src/Symfony/Component/Image/README.md

@@ -0,0 +1,3 @@
+Symfony Image Component
+=======================
+


The readme is incomplete here. It should have the basic info we have in other components

stof · 2017-03-28T08:25:18Z

src/Symfony/Component/Image/Tests/Constraint/IsImageEqual.php

+        }
+
+        if (!is_numeric($delta)) {
+            throw \PHPUnit_Util_InvalidArgumentHelper::factory(2, 'numeric');


this won't work on PHPUnit 6, as the non-namespaced class does not exist anymore

stof · 2017-03-28T08:26:37Z

src/Symfony/Component/Image/Tests/Draw/AbstractDrawerTest.php

+
+        $canvas->save(__DIR__.'/../results/smiley.png');
+
+        $this->assertTrue(file_exists(__DIR__.'/../results/smiley.png'));


Use assertFileExists

stof · 2017-03-28T08:29:43Z

src/Symfony/Component/Image/Tests/Issues/Issue59Test.php

+use Symfony\Component\Image\Exception\RuntimeException;
+use Symfony\Component\Image\Tests\TestCase;
+
+class Issue59Test extends TestCase


Tests should be renamed, as referencing Imagine issue numbers does not make sense

stof · 2017-03-28T08:30:30Z

src/Symfony/Component/Image/Tests/Draw/AbstractDrawerTest.php

+            ->ellipse(new Point(125, 100), new Box(50, 50), $this->getColor('fff'))
+            ->ellipse(new Point(275, 100), new Box(50, 50), $this->getColor('fff'), true);
+
+        $canvas->save(__DIR__.'/../results/smiley.png');


you should either use a path outside the repo (in the system temp dir) or mark this folder as ignored.

romainneutron · 2017-03-28T10:34:16Z

I think I've addressed all your comments. I've added a BC layer with deprecation for all options (reolution-* => resolution_*, quality, filters, resampling-filter)

teohhanhui · 2017-04-03T08:08:20Z

If Symfony really wants to introduce an Image component, it'd be great if it uses libvips. PHP binding here: https://github.com/jcupitt/php-vips

A wonderful library based on libvips in the Node.js world: https://github.com/lovell/sharp

robfrawley · 2017-04-18T15:56:28Z

Note that we may want to work on the brittle aspects of the unit tests before introducing this component: the Travis CI tests break regularly as the various extensions mature due to slightly different result values as well as deprecated and/or removed methods/constants/etc. Can we do something to mitigate these issues before adding this to the Symfony test runtime?

carsonbot added Status: Needs Review Bug Feature BC Break Deprecation labels Mar 1, 2017

romainneutron removed BC Break Bug Deprecation labels Mar 1, 2017

romainneutron force-pushed the image-component branch from ffc201d to 336c887 Compare March 1, 2017 17:14

romainneutron force-pushed the image-component branch from 336c887 to dcd13b3 Compare March 1, 2017 17:36

romainneutron force-pushed the image-component branch 3 times, most recently from 4ee00c7 to 82598b6 Compare March 1, 2017 17:52

robfrawley mentioned this pull request Mar 1, 2017

Image transformation library proxy liip/LiipImagineBundle#890

Closed

lsmith77 mentioned this pull request Mar 2, 2017

Roadmap 2.0 liip/LiipImagineBundle#686

Closed

9 tasks

romainneutron force-pushed the image-component branch 6 times, most recently from e6d05fb to 44c814e Compare March 27, 2017 18:12

romainneutron force-pushed the image-component branch 2 times, most recently from 9765c31 to d2e7f46 Compare March 28, 2017 07:11

stof requested changes Mar 28, 2017

View reviewed changes

Add Symfony Image Component

916aff8

romainneutron force-pushed the image-component branch 5 times, most recently from d28942c to 96451e7 Compare March 28, 2017 09:45

Integrate new Image Component

a80d5f0

romainneutron force-pushed the image-component branch from 96451e7 to a80d5f0 Compare March 28, 2017 10:31

nicolas-grekas modified the milestones: 3.3, 3.4 Apr 11, 2017

mickaelandrieu mentioned this pull request Jun 20, 2017

Possibly missing PHP extension in System Requirements akeneo/pim-docs#565

Closed

fabpot mentioned this pull request Jun 28, 2017

Added two new components: AMQP and Worker #23315

Closed

5 tasks

nicolas-grekas modified the milestones: 3.4, 4.1 Sep 28, 2017

fabpot closed this Feb 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Symfony Image Component #21820

Add Symfony Image Component #21820

avalanche123 commented Mar 1, 2017 •

edited by romainneutron

romainneutron commented Mar 1, 2017

stof commented Mar 1, 2017

fabpot commented Mar 1, 2017

avalanche123 commented Mar 1, 2017

nicolas-grekas commented Mar 1, 2017 •

edited

fabpot commented Mar 1, 2017

stof commented Mar 1, 2017

ausi commented Mar 1, 2017

Seldaek commented Mar 1, 2017

andrerom commented Mar 2, 2017

fbourigault commented Mar 2, 2017

javiereguiluz commented Mar 2, 2017

nicolas-grekas commented Mar 2, 2017

ausi commented Mar 2, 2017

sstok commented Mar 2, 2017

romainneutron commented Mar 27, 2017

stof Mar 28, 2017

stof Mar 28, 2017

stof Mar 28, 2017

stof Mar 28, 2017

stof Mar 28, 2017

stof Mar 28, 2017

stof Mar 28, 2017

romainneutron Mar 28, 2017

stof Mar 28, 2017

stof Mar 28, 2017

stof Mar 28, 2017

stof Mar 28, 2017

romainneutron commented Mar 28, 2017

teohhanhui commented Apr 3, 2017

robfrawley commented Apr 18, 2017

		@@ -0,0 +1,3 @@
		Symfony Image Component
		=======================


		$canvas->save(__DIR__.'/../results/smiley.png');

		$this->assertTrue(file_exists(__DIR__.'/../results/smiley.png'));

Add Symfony Image Component #21820

Add Symfony Image Component #21820

Conversation

avalanche123 commented Mar 1, 2017 • edited by romainneutron

romainneutron commented Mar 1, 2017

stof commented Mar 1, 2017

fabpot commented Mar 1, 2017

avalanche123 commented Mar 1, 2017

nicolas-grekas commented Mar 1, 2017 • edited

fabpot commented Mar 1, 2017

stof commented Mar 1, 2017

ausi commented Mar 1, 2017

Seldaek commented Mar 1, 2017

andrerom commented Mar 2, 2017

fbourigault commented Mar 2, 2017

javiereguiluz commented Mar 2, 2017

nicolas-grekas commented Mar 2, 2017

ausi commented Mar 2, 2017

sstok commented Mar 2, 2017

romainneutron commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romainneutron commented Mar 28, 2017

teohhanhui commented Apr 3, 2017

robfrawley commented Apr 18, 2017

avalanche123 commented Mar 1, 2017 •

edited by romainneutron

nicolas-grekas commented Mar 1, 2017 •

edited