Automatically tag and register filters

[homersimpsons]

Is your feature request related to a problem? Please describe.
Using custom filters is pretty verbose, you have to:

1. Extend the Liip\ImagineBundle\Imagine\Filter\Loader\LoaderInterface
2. Create a service in your services.yaml
3. Tag the service with the liip_imagine.filter.loader
4. Add a loader property to the tag that will then be referred in the filter_sets

Describe the solution you'd like
Using a proper symfony compiler pass would reduce this to only the first step (1. Extend the Liip\ImagineBundle\Imagine\Filter\Loader\LoaderInterface)

In fact for (2.) and (3.), the service could be tagged automatically:

$container->registerForAutoconfiguration(LoaderInterface::class)->addTag('liip_imagine.filter.loader');

And for (4.) we could use the service_id:

$tags = $container->findTaggedServiceIds('liip_imagine.filter.loader');
foreach ($tags as $serviceId => $tags) {
    $manager->addMethodCall('addLoader', [$serviceId, new Reference($serviceId)]);
}

Describe alternatives you've considered
~

Additional context
~

[homersimpsons]

For the record I did this quick development in my project:

// src/Kernel.php
namespace App;

use Liip\ImagineBundle\Imagine\Filter\Loader\LoaderInterface;
use Symfony\Bundle\FrameworkBundle\Kernel\MicroKernelTrait;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\Compiler\PassConfig;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpKernel\Kernel as BaseKernel;

class Kernel extends BaseKernel implements CompilerPassInterface
{
    use MicroKernelTrait;

    private const TEMPORARY_LIIP_IMAGINE_FILTER_TAG = 'temp.liip_imagine.filter';

    protected function build(ContainerBuilder $container)
    {
        parent::build($container);

        $container->registerForAutoconfiguration(LoaderInterface::class)->addTag(self::TEMPORARY_LIIP_IMAGINE_FILTER_TAG);
        // Should add myself as a compiler pass so we can increase priority and happen before LiipImagineBundle's CompilerPass
        $container->addCompilerPass($this, PassConfig::TYPE_BEFORE_OPTIMIZATION, 1);
    }

    public function process(ContainerBuilder $container): void
    {
        $servicesWithTags = $container->findTaggedServiceIds(self::TEMPORARY_LIIP_IMAGINE_FILTER_TAG);
        foreach ($servicesWithTags as $serviceId => $tags) {
            $definition = $container->getDefinition($serviceId);
            $definition->clearTag(self::TEMPORARY_LIIP_IMAGINE_FILTER_TAG);
            $definition->addTag('liip_imagine.filter.loader', ['loader' => $serviceId]);
        }
    }
}

[homersimpsons]

I just noticed that post processors have the same verbosity.

[dbu]

hi @homersimpsons , thanks for reporting this. i think you are right that we should improve on this. (this bundle is much older than the autoconfiguration capabilities of symfony). glad if you want to do a pull request to set up autoconfiguration. when using autoconfiguration would mean having to use the class name in the configuration, right? which would be fine for php configuration, and if people want a nice name, they can still define the filter as service, and optionally add the tag to set the loader attribute.

[homersimpsons]

glad if you want to do a pull request to set up autoconfiguration

I did open #1486 as a draft but I would like to go further (maybe for 3.x instead of 2.x ?). I wanted to wait for approval before refactoring this.

when using autoconfiguration would mean having to use the class name in the configuration, right?

Exactly, with the above code I can do the following configuration:

1440x960:
    filters:
        auto_rotate: ~
        App\Filter\FocalCrop: { size: [1440, 960] }
        App\Filter\Watermark: { image: '%kernel.project_dir%/public/default/watermark.png', position: center, size: [297, 150] }
    data_loader: loader0

and if people want a nice name, they can still define the filter as service

Exactly

and optionally add the tag to set the loader attribute

I guess we could get rid of this loader attribute completely. In my mind, it adds useless indirection to the implementing class.

Doing so the above example would become:

1440x960:
    filters:
        Liip\ImagineBundle\Imagine\Filter\Loader\AutoRotateFilterLoader: ~
        App\Filter\FocalCrop: { size: [1440, 960] }
        App\Filter\Watermark: { image: '%kernel.project_dir%/public/default/watermark.png', position: center, size: [297, 150] }
    data_loader: loader0

Full proposition

Affected services

To me, the change to use service name should be applied to:

• Filter (implements Liip\ImagineBundle\Imagine\Filter\Loader\LoaderInterface)
• PostProcessor (implements Liip\ImagineBundle\Imagine\Filter\PostProcessor\PostProcessorInterface)
• DataLoader (implements Liip\ImagineBundle\Binary\Loader\LoaderInterface)
• Other ?

Do you agree on the above ?

Service id as the only reference

The ideal would be to only use service name, and to automatically create services using their class' Fully Qualified Class Name
as service id.

Doing so would require everyone to update its configuration so maybe this should target 3.x and a deprecation should be emitted in 2.x. We could alias services to their old name to ease the transition maybe.

This would also heavily reduce the bundle internal configuration.

Should the change keep only service ids, or should we still offer the loader configuration ?

Update documentation and migration guide

The whole documentation will require update according to the above changes. Also a migration guide may need to be written (changes would be basically search and replace)

[dbu]

🤔 good ideas. using the class names would be in line with e.g. how forms has been changed. if we use service ids, it would still work to define an alias for a service if somebody prefers a short name, right? if that works, i agree to drop the whole loader alias names for 3.x

i think the correct way forward is to at least provide the option to directly specify service ids in the 2.x branch and deprecate using the loader name indirection, and then in version 3 we can drop the whole loader thing.

the autowiring can also go in 2.x if you want, we don't have a clear release date for 3.x yet - it is mainly about deleting deprecated things and strict typeing which is a BC break if people implemented interfaces or extended classes.