Allow to configure the imagine class #1615

Toflar · 2018-07-11T15:00:27Z

Right now you cannot configure the desired Imagine class. This has two major drawbacks:

If e.g. Imagick is installed on your system but it's broken somehow, you are lost. You cannot switch to Gd without adding another compiler pass.
If you add your own Imagine implementation you cannot use that without adding another compiler pass.

This PR allows you to configure the class very easily:

contao:
    image:
        imagine_service: 'contao.image.imagine.gd'

m-vo · 2018-07-11T16:01:17Z

src/DependencyInjection/Configuration.php

+            // Will throw an exception if the PHP implementation is not available
+            try {
+                new $class();
+            } catch (RuntimeException $e) {


What's the benefit of catching instead of letting it fail?

It will fall back to return Imagine::class; // see #616

ausi · 2018-07-11T16:16:31Z

src/DependencyInjection/Configuration.php

@@ -145,6 +147,9 @@ function (string $value): string {
                        ->arrayNode('imagine_options')
                            ->addDefaultsIfNotSet()
                            ->children()
+                                ->scalarNode('class')


The imagine_options array gets directly passed to setImagineOptions() https://github.com/contao/image/blob/dc9e2dc78533c6da1d7d0cf010b0b89b2b5ffd06/src/ResizeOptionsInterface.php#L29 so we should probably not add the class name here.

IMO it would be better to use contao.image.imagine_class instead.

ausi · 2018-07-11T16:25:20Z

I’m not sure if this is the correct approach. How about keeping the existing compiler pass, removing the class from the services.yml in

core-bundle/src/Resources/config/services.yml

Line 148 in 864fd03

class: Imagine\Gd\Imagine

and checking for ->getClass() === null?

public function process(ContainerBuilder $container): void
{
    $definition = $container->getDefinition('contao.image.imagine');
    
    if (null === $definition->getClass()) {
        $definition->setClass($this->getImagineImplementation());
    }
}

This way you can simply overwrite the used class in the service definition in your installation.

What do you think?

aschempp · 2018-07-11T16:34:45Z

@ausi 's suggestion looks reasonable to me

Toflar · 2018-07-12T08:13:02Z

Nope, I disagree here. The compiler pass was wrong from day one that's why I changed it. The class you would like to use is a configuration value. It's exactly the right place to do so and it doesn't need any compiler pass at all. But yes, I can change the key to contao.image.imagine_class.

ausi · 2018-07-12T12:42:31Z

But what if the custom Imagine class you want to use needs a constructor parameter or other injections?

IMO the services definition ist the right place for replacing services in your application.

aschempp · 2018-07-12T12:47:31Z

But what if the custom Imagine class you want to use needs a constructor parameter or other injections?

That means you would need dependency injection and not just set the class as an argument. A completely new (maybe valid) case. Maybe we should generate a service on the fly and assign it as argument to that method?

Toflar · 2018-07-12T12:54:51Z

I have a better idea, give me a few minutes :)

Toflar · 2018-07-12T13:38:50Z

Okay, so now you can configure any service you like. This will allow to define your own service in the container (that has other arguments) and configure it here. If you don't configure anything at all, the best solution is chosen for you and aliased.
For convenience, I've added gd, imagick and gmagick so it's easy to configure if you want to use one of these three, see the updated initial example.

leofeyer · 2018-07-12T13:41:18Z

src/DependencyInjection/Configuration.php

@@ -142,6 +142,10 @@ function (string $value): string {
                            ->prototype('scalar')->end()
                            ->defaultValue(['jpg', 'jpeg', 'gif', 'png', 'tif', 'tiff', 'bmp', 'svg', 'svgz'])
                        ->end()
+                        ->scalarNode('imagine_service')
+                            ->info('By default Contao tries to take the best Imagine service out of Gmagick, Imagick and Gd (in this order). If you would like to use a different service, configure the service id here.')


When will this be displayed?

config:dump-reference :)

Nice. However we don't have this for any other parameter, therefore I would prefer to remove this from this PR and to create a separate PR which adds descriptions for all parameters. WDYT?

Nah, it belongs to this PR. Just add the info in another commit :)

leofeyer · 2018-07-12T14:01:49Z

Ok. Let's see what @ausi says and then merge this.

ausi · 2018-07-12T14:37:30Z

I still think that the services definition is the right place for replacing services in your application. We do that in Contao for example with the translator service. Every developer that is familiar with Symfony knows how to do that:

services:
    contao.image.imagine:
        class: Imagine\Gd\Imagine

To get knowledge of the configuration value you need to read the documentation first.

My suggestion from #1615 (comment) would solve your issues too, right?

Toflar · 2018-07-12T14:39:04Z

I don't like that. I don't want to configure things in my services.yml. I already configure the imagine options in contao.image. So I would need to do things in 2 places.

ausi · 2018-07-12T14:44:11Z

But how do you configure which translator class should be used be the Symfony translator for example, or any other service?

Toflar · 2018-07-12T14:50:36Z

No idea, it's not part of my problem at the moment 😄 The way this PR implements the service you choose is the same as it's done in thousands of bundles. I don't see the issue here. There's always many ways to achieve the same thing but this one is the one I like best.

aschempp · 2018-07-12T14:51:21Z

That is not the same to me. Overriding the application is one thing, setting configuration is a different thing.

Manually defining your Imagine class is a configuration that makes sense, because people might prefer one over the other. Therefore, there should be a configuration so set it. It's like settings the JPEG quality, you could do that using a compiler pass and change the service argument, but that's just unnecessarily cumbersome. We want people to configure their Imagine service, if they know which one they can use on their server.

Same as with the Doctrine server version. You should configure it, but if you don't the system tries to detect the current version.

ausi

K, I’m convinced then 🙃

leofeyer · 2018-07-16T07:04:00Z

Thank you @Toflar.

…#1615) Description ----------- See https://developers.google.com/analytics/devguides/collection/analyticsjs/cookie-usage Commits ------- 08212655 Add Google Analytics ga.js Cookies to StripCookiesSubscriber 3716fcde Update StripCookiesSubscriber.php a05ff52c Update StripCookiesSubscriber.php

Allow to configure the imagine class

da33b10

leofeyer assigned Toflar Jul 11, 2018

leofeyer added the feature label Jul 11, 2018

leofeyer added this to the 4.6.0 milestone Jul 11, 2018

m-vo reviewed Jul 11, 2018

View reviewed changes

ausi requested changes Jul 11, 2018

View reviewed changes

Changed config option name

24c3238

Allow to define an arbitrary service to be used

eed5899

leofeyer reviewed Jul 12, 2018

View reviewed changes

CS

f0be6c7

aschempp approved these changes Jul 12, 2018

View reviewed changes

ausi approved these changes Jul 12, 2018

View reviewed changes

Fix the coding style.

1f1243a

leofeyer merged commit 89a9201 into contao:master Jul 16, 2018

Toflar deleted the configurable-imagine-class branch July 16, 2018 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to configure the imagine class #1615

Allow to configure the imagine class #1615

Toflar commented Jul 11, 2018 •

edited

Loading

m-vo Jul 11, 2018

bytehead Jul 11, 2018

ausi Jul 11, 2018

aschempp Jul 11, 2018 •

edited

Loading

ausi commented Jul 11, 2018

aschempp commented Jul 11, 2018

Toflar commented Jul 12, 2018

ausi commented Jul 12, 2018

aschempp commented Jul 12, 2018

Toflar commented Jul 12, 2018

Toflar commented Jul 12, 2018

leofeyer Jul 12, 2018

Toflar Jul 12, 2018

leofeyer Jul 12, 2018

Toflar Jul 12, 2018

leofeyer commented Jul 12, 2018

ausi commented Jul 12, 2018

Toflar commented Jul 12, 2018

ausi commented Jul 12, 2018

Toflar commented Jul 12, 2018

aschempp commented Jul 12, 2018 •

edited

Loading

ausi left a comment

leofeyer commented Jul 16, 2018

Allow to configure the imagine class #1615

Allow to configure the imagine class #1615

Conversation

Toflar commented Jul 11, 2018 • edited Loading

m-vo Jul 11, 2018

Choose a reason for hiding this comment

bytehead Jul 11, 2018

Choose a reason for hiding this comment

ausi Jul 11, 2018

Choose a reason for hiding this comment

aschempp Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

ausi commented Jul 11, 2018

aschempp commented Jul 11, 2018

Toflar commented Jul 12, 2018

ausi commented Jul 12, 2018

aschempp commented Jul 12, 2018

Toflar commented Jul 12, 2018

Toflar commented Jul 12, 2018

leofeyer Jul 12, 2018

Choose a reason for hiding this comment

Toflar Jul 12, 2018

Choose a reason for hiding this comment

leofeyer Jul 12, 2018

Choose a reason for hiding this comment

Toflar Jul 12, 2018

Choose a reason for hiding this comment

leofeyer commented Jul 12, 2018

ausi commented Jul 12, 2018

Toflar commented Jul 12, 2018

ausi commented Jul 12, 2018

Toflar commented Jul 12, 2018

aschempp commented Jul 12, 2018 • edited Loading

ausi left a comment

Choose a reason for hiding this comment

leofeyer commented Jul 16, 2018

Toflar commented Jul 11, 2018 •

edited

Loading

aschempp Jul 11, 2018 •

edited

Loading

aschempp commented Jul 12, 2018 •

edited

Loading