Open homersimpsons opened 2 years ago
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]);
}
}
}
I just noticed that post processors have the same verbosity.
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.
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
To me, the change to use service name should be applied to:
implements Liip\ImagineBundle\Imagine\Filter\Loader\LoaderInterface
)implements Liip\ImagineBundle\Imagine\Filter\PostProcessor\PostProcessorInterface
)implements Liip\ImagineBundle\Binary\Loader\LoaderInterface
)Do you agree on the above ?
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 ?
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)
:thinking: 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.
Is your feature request related to a problem? Please describe. Using custom filters is pretty verbose, you have to:
Liip\ImagineBundle\Imagine\Filter\Loader\LoaderInterface
services.yaml
liip_imagine.filter.loader
loader
property to the tag that will then be referred in thefilter_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:
And for (4.) we could use the service_id:
Describe alternatives you've considered ~
Additional context ~