[RTM] Image and Picture as services #342

ausi · 2015-09-08T20:05:35Z

This is a very early draft of the image handling as a service. Here are my ideas:

The new classes Contao\CoreBundle\Image\Image and Contao\CoreBundle\Image\Picture work similar to the current classes, but with dependency injection and without the create and legacy methods. The new Image class uses Imagine to manipulate image data. The Image and Picture objects are created via a factory which is registered in the service container.

The old classes Contao\Image and Contao\Picture are wrappers around the new classes and services to keep backwards compatibility.

Does that make sense?

Questions

How do we handle the the hooks executeResize and getImage? To be 100% backwards compatible we would have to pass an instance of the Contao\Image class.
Should we still use the Contao\File class?
ImageFactory and PictureFactory or Factory with createImage and createPicture methods?
Should we split the functionality of how the replaced images are saved/cached into a separate contao.image.cache service?
Is the imagine_class configuration node the right way to make the used Imagine class configurable?
We would need something like a contao.adapter.model_repository service to get access to the files and image size models.

aschempp · 2015-09-08T20:19:58Z

To tell you my opinion on the basic questions:

we should not use Contao\File
Two factories is better, each should only have one create method
The contao.image.imagine service looks pretty valid to me

Regarding the model_repository, I guess you misunderstood me on that. Each class which currently has a static method should get it's own adapter. So in your case you would probably need contao.adapter.files_model and contao.adapter.image_sizes services.

Also, we should use a compiler pass to automatically detect the available Imagine options and choose the best (= only fallback to GD2 if Imagick is not supported).

Toflar · 2015-09-09T06:51:43Z

Regarding the model_repository, I guess you misunderstood me on that. Each class which currently has a static method should get it's own adapter. So in your case you would probably need contao.adapter.files_model and contao.adapter.image_sizes services.

Thinking about this, I think a general model_repository would be more intelligent. It would also cover non-core models.

ausi · 2015-09-09T07:47:04Z

Thinking about this, I think a general model_repository would be more intelligent. It would also cover non-core models.

For the image service we would have to inject three models: ImageSizeModel, ImageSizeItemModel and FilesModel. Therefore I think a general model_repository would make it cleaner.

aschempp · 2015-09-09T07:52:43Z

Just think about Doctrine. Each entity has it's repository…

Toflar · 2015-09-09T07:55:31Z

but only one entity manager ;)

ausi · 2015-09-09T07:56:57Z

But I can inject doctrine and call ->getRepository('...') on it. IMO it would be great to have a single service to get any Contao model from.

Toflar · 2015-09-09T08:31:24Z

But I can inject doctrine and call ->getRepository('...') on it. IMO it would be great to have a single service to get any Contao model from.

👍

aschempp · 2015-09-09T08:59:30Z

But I can inject doctrine and call ->getRepository('...') on it. IMO it would be great to have a single service to get any Contao model from.

True! It just means we will improve our model stuff while we actually wanted to prefer Doctrine ^^

ausi · 2015-09-09T09:04:34Z

Also, we should use a compiler pass to automatically detect the available Imagine options and choose the best (= only fallback to GD2 if Imagick is not supported).

Like so aaf12ec?

ausi · 2015-09-09T09:14:10Z

we should not use Contao\File

Where should we put the code for Contao\File::__get('imageSize') then? Into the new Image class itself?

ausi · 2015-09-09T12:12:07Z

Can I start working on it with the class structure described above?
Or do we need to discuss something before I start?

aschempp · 2015-09-10T11:36:06Z

I would love to discuss the topic on today's Contao call. The compiler pass looks pretty good.

ausi · 2015-09-10T11:45:31Z

I will be there.

ausi · 2015-09-29T16:16:57Z

I added some classes :)

The current proposal would work like this:

$imageFactory->create('foo.jpg') creates an Image and returns it.

$imageFactory->create('foo.jpg', 2) creates an Image, fetches the image size 2 from the database and passes the data to the Resizer. The Resizer passes the configuration to the ResizeCalculator and resizes the image according to the result and returns it.

$pictureFactory->create('foo.jpg', 2) creates an Image, fetches the image size 2 from the database and passes the data to the PictureGenerator. The PictureGenerator uses the Resizer to resize the images and returns a Picture object with the results.

@contao/developers what do you think?

aschempp · 2015-09-29T17:25:53Z

src/DependencyInjection/Compiler/DetermineImagineImplementation.php

+
+            try {
+                new $class();
+                break;


I just realized this can be simplified a lot. Simply return $class here, then you also don't need the if-condition below (just throw the exception).

Toflar · 2015-09-29T17:38:36Z

I think I like the basic approach :-) The only thing I would change to bring even more interoperability would be to use things like ResizerInterface and type hint this instead of a fixed dependency on Resizer so we can easily exchange those :)

ausi · 2015-09-29T17:41:28Z

@Toflar thanks. I agree we should add some interfaces, but I would like to get a working implementation and then adding the interfaces, to better understand which interfaces could be useful.

aschempp · 2015-09-29T17:47:13Z

src/Image/ImageDimensions.php

+        $relative = false
+    ) {
+        $this->size = $size;
+        $this->relative = (bool) $relative;


What is a relative image dimension?

For images with a ratio, but no absolute size. E.g. a SVG image with width="100%" height="100%".
See the difference between File->imageSize and File->imageViewSize in contao/core#7882.

aschempp · 2015-09-29T17:54:45Z

I also like the approach in general. I just wonder if the Image class should be mutable at all? Also, I think the factory should not accept an ID of an image configuration (from DB) but directly it's configuration.

ausi · 2015-09-29T18:04:55Z

I just wonder if the Image class should be mutable at all?

You suggest to add $importantPart as a constructor argument?
I tried to follow the approach to add required attributes to the constructors and optional attributes as setters.

I think the factory should not accept an ID of an image configuration (from DB) but directly it's configuration.

Where should we put the code for that than?

aschempp · 2015-10-22T20:45:24Z

Now that the adapters are finally merged, I think we can continue with that?

ausi · 2016-07-23T18:19:17Z

This should be ready for comments now :)

One thing I’d like to discuss before merging is, what should be the Symfony-way alternative to the image hooks executeResize and getImage? In my project I can now easily replace one of the services to add custom behavior, but how can I do that as a Contao bundle?

Note to myself: After this gets merged I should take a look at some other image related feature requests:

Add support for w descriptors Add support for w descriptors in densities setting image#1
~~Add percentage important part to Image sizes Imagesize: add resize-mode options core#8360~~
~~Image Size alias Create Image Size alias core#7520~~
CSS classes for image sizes Ansichtsproblem auf 5k Display in Firefox 43.0.4 & Safari 9.0.3 core#8202
Default densities in theme settings automatically use pre-defined densities when none given core#8171

…tory::create

Conflicts: src/Resources/contao/dca/tl_settings.php

This reverts commit 386fe86. Conflicts: src/Image/ImageFactory.php src/Image/PictureFactory.php tests/Image/ImageFactoryTest.php tests/Image/PictureFactoryTest.php

leofeyer · 2016-07-29T11:55:19Z

src/DependencyInjection/Compiler/AddRootDirPass.php

+    public function process(ContainerBuilder $container)
+    {
+        $container->setParameter(
+            'contao.root_dir',


This code does not what the phpDoc comment says. Also, we should not add a contao.root_dir parameter.

We need a way to inject "%contao.root_dir%/%contao.image.target_path%" into the resizer service. Should I use the %kernel.root_dir% instead?

This is not so easy. I have created a working implementation, but I need to discuss this with you on Mumble.

leofeyer · 2016-08-04T15:02:16Z

Merged in 9b6694f.

ausi changed the title ~~Image and Picture as services~~ [WIP] Image and Picture as services Sep 8, 2015

leofeyer added the feature label Sep 9, 2015

leofeyer added this to the 4.1.0 milestone Sep 9, 2015

leofeyer assigned ausi Sep 9, 2015

aschempp reviewed Sep 29, 2015
View reviewed changes

ausi mentioned this pull request Oct 1, 2015

[RTM] Added a general adapter factory #346

Merged

1 task

ausi added 5 commits July 22, 2016 19:56

Use interfaces from Contao\Image

0e746b5

Fix ImageFactory test

6925fb9

Remove root directory from picture generator

eafe848

Fix typo

180a97c

Use ResizeOptions in PictureGenerator

1cca8a0

ausi changed the title ~~[WIP] Image and Picture as services~~ [RFC] Image and Picture as services Jul 23, 2016

ausi changed the title ~~[RFC] Image and Picture as services~~ [RTM] Image and Picture as services Jul 25, 2016

This was referenced Jul 26, 2016

Add CSS class to image sizes #553

Closed

[RTM] Add default image densities to theme settings #554

Merged

[RTM] Add CSS class to image sizes #555

Merged

leofeyer added this to the 4.3.0 milestone Jul 28, 2016

ausi mentioned this pull request Jul 28, 2016

Split the classes into namespaces contao/image#3

Merged

ausi added 7 commits July 28, 2016 23:52

Update class namespaces of Contao\Image

386fe86

Update the pixel densities description

1b40d9b

Allow Image objects as first param for ImageFactory::create

af230b9

Allow Image and PictureConfiguration objects as params for PictureFac…

5e19e3e

…tory::create

Merge branch origin/develop into feature/image-services

21dc13a

Conflicts: src/Resources/contao/dca/tl_settings.php

Add interfaces for ImageFactory and PictureFactory

540c664

Revert "Update class namespaces of Contao\Image"

a9fb428

This reverts commit 386fe86. Conflicts: src/Image/ImageFactory.php src/Image/PictureFactory.php tests/Image/ImageFactoryTest.php tests/Image/PictureFactoryTest.php

leofeyer reviewed Jul 29, 2016
View reviewed changes

leofeyer merged commit a9fb428 into contao:develop Aug 4, 2016

leofeyer self-assigned this Aug 4, 2016

ausi mentioned this pull request Aug 24, 2016

Already used by contao 4 contao/image#17

Closed

leofeyer mentioned this pull request Aug 24, 2016

Thumbnails should save the data of Exif and IPTC contao/core#8446

Open

ausi mentioned this pull request Oct 9, 2017

Definition von Bildgrößen umständlich #1138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTM] Image and Picture as services #342

[RTM] Image and Picture as services #342

ausi commented Sep 8, 2015 •

edited

aschempp commented Sep 8, 2015

Toflar commented Sep 9, 2015

ausi commented Sep 9, 2015

aschempp commented Sep 9, 2015

Toflar commented Sep 9, 2015

ausi commented Sep 9, 2015

Toflar commented Sep 9, 2015

aschempp commented Sep 9, 2015

ausi commented Sep 9, 2015

ausi commented Sep 9, 2015

ausi commented Sep 9, 2015

aschempp commented Sep 10, 2015

ausi commented Sep 10, 2015

ausi commented Sep 29, 2015

aschempp Sep 29, 2015

ausi Sep 29, 2015

Toflar commented Sep 29, 2015

ausi commented Sep 29, 2015

aschempp Sep 29, 2015

ausi Sep 29, 2015

aschempp commented Sep 29, 2015

ausi commented Sep 29, 2015

aschempp commented Oct 22, 2015

ausi commented Jul 23, 2016 •

edited

leofeyer Jul 29, 2016

ausi Jul 29, 2016

leofeyer Jul 29, 2016

leofeyer commented Aug 4, 2016

[RTM] Image and Picture as services #342

[RTM] Image and Picture as services #342

Conversation

ausi commented Sep 8, 2015 • edited

Questions

aschempp commented Sep 8, 2015

Toflar commented Sep 9, 2015

ausi commented Sep 9, 2015

aschempp commented Sep 9, 2015

Toflar commented Sep 9, 2015

ausi commented Sep 9, 2015

Toflar commented Sep 9, 2015

aschempp commented Sep 9, 2015

ausi commented Sep 9, 2015

ausi commented Sep 9, 2015

ausi commented Sep 9, 2015

aschempp commented Sep 10, 2015

ausi commented Sep 10, 2015

ausi commented Sep 29, 2015

aschempp Sep 29, 2015

Choose a reason for hiding this comment

ausi Sep 29, 2015

Choose a reason for hiding this comment

Toflar commented Sep 29, 2015

ausi commented Sep 29, 2015

aschempp Sep 29, 2015

Choose a reason for hiding this comment

ausi Sep 29, 2015

Choose a reason for hiding this comment

aschempp commented Sep 29, 2015

ausi commented Sep 29, 2015

aschempp commented Oct 22, 2015

ausi commented Jul 23, 2016 • edited

leofeyer Jul 29, 2016

Choose a reason for hiding this comment

ausi Jul 29, 2016

Choose a reason for hiding this comment

leofeyer Jul 29, 2016

Choose a reason for hiding this comment

leofeyer commented Aug 4, 2016

ausi commented Sep 8, 2015 •

edited

ausi commented Jul 23, 2016 •

edited