Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defaults to service id over names #1486

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DependencyInjection/Compiler/FiltersCompilerPass.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public function process(ContainerBuilder $container): void
$manager = $container->getDefinition('liip_imagine.filter.manager');

foreach ($tags as $id => $tag) {
$manager->addMethodCall('addLoader', [$tag[0]['loader'], new Reference($id)]);
$manager->addMethodCall('addLoader', [$tag[0]['loader'] ?? $id, new Reference($id)]);
$this->log($container, 'Registered filter loader: %s', $id);
}
}
Expand Down
2 changes: 1 addition & 1 deletion DependencyInjection/Compiler/LoadersCompilerPass.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public function process(ContainerBuilder $container): void
$manager = $container->getDefinition('liip_imagine.data.manager');

foreach ($tags as $id => $tag) {
$manager->addMethodCall('addLoader', [$tag[0]['loader'], new Reference($id)]);
$manager->addMethodCall('addLoader', [$tag[0]['loader'] ?? $id, new Reference($id)]);
$this->log($container, 'Registered binary loader: %s', $id);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public function process(ContainerBuilder $container): void
$manager = $container->getDefinition('liip_imagine.filter.manager');

foreach ($tags as $id => $tag) {
$manager->addMethodCall('addPostProcessor', [$tag[0]['post_processor'], new Reference($id)]);
$manager->addMethodCall('addPostProcessor', [$tag[0]['post_processor'] ?? $id, new Reference($id)]);
$this->log($container, 'Registered filter post-processor: %s', $id);
}
}
Expand Down
7 changes: 7 additions & 0 deletions LiipImagineBundle.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Enqueue\Bundle\DependencyInjection\Compiler\AddTopicMetaPass;
use Liip\ImagineBundle\Async\Topics;
use Liip\ImagineBundle\Binary\Loader\LoaderInterface as BinaryLoaderInterface;
use Liip\ImagineBundle\DependencyInjection\Compiler\AssetsVersionCompilerPass;
use Liip\ImagineBundle\DependencyInjection\Compiler\DriverCompilerPass;
use Liip\ImagineBundle\DependencyInjection\Compiler\FiltersCompilerPass;
Expand All @@ -30,6 +31,8 @@
use Liip\ImagineBundle\DependencyInjection\Factory\Resolver\FlysystemResolverFactory;
use Liip\ImagineBundle\DependencyInjection\Factory\Resolver\WebPathResolverFactory;
use Liip\ImagineBundle\DependencyInjection\LiipImagineExtension;
use Liip\ImagineBundle\Imagine\Filter\Loader\LoaderInterface as LoaderLoaderInterface;
use Liip\ImagineBundle\Imagine\Filter\PostProcessor\PostProcessorInterface;
use Symfony\Component\DependencyInjection\Compiler\PassConfig;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpKernel\Bundle\Bundle;
Expand Down Expand Up @@ -67,5 +70,9 @@ public function build(ContainerBuilder $container): void
$extension->addLoaderFactory(new FileSystemLoaderFactory());
$extension->addLoaderFactory(new FlysystemLoaderFactory());
$extension->addLoaderFactory(new ChainLoaderFactory());

$container->registerForAutoconfiguration(LoaderLoaderInterface::class)->addTag('liip_imagine.filter.loader');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is only available since 5.3 - can you check the symfony version before calling it?

the 5.4 build and lowest version builds fail - not sure if they install a too old symfony or if there is something else as well that we need to do for the test to work on legacy versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd also be ok to only do it with symfony 6 or 7 and otherwise skip.

the doc should mention from what symfony version on this works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did update the code to check if the function exists and updated the doc too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well actually it looks like this was introduced a while ago looking at this commit symfony/symfony@18627bf

But here it is returning null which is rather strange as the first implementation always return a Symfony\Component\DependencyInjection\ChildDefinition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay, this is because we are using a MockObject in place of a real container in the test.

Copy link
Contributor Author

@homersimpsons homersimpsons Jan 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did revert my changes and add the method to the mock which should fix the tests.

Note that the setMethods mock method is removed from PHPUnit sebastianbergmann/phpunit#3687 (comment). Also I'm wondering why we have to create such mocks at first I think we can rely on the ContainerBuilder directly (like in https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php)

$container->registerForAutoconfiguration(PostProcessorInterface::class)->addTag('liip_imagine.filter.post_processor');
$container->registerForAutoconfiguration(BinaryLoaderInterface::class)->addTag('liip_imagine.binary.loader');
}
}
12 changes: 11 additions & 1 deletion Resources/doc/data-loaders-custom.rst
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,17 @@ path to the image and needs to return an instance of ``BinaryInterface``.
to sanitize this parameter in your loader to avoid exposing files outside
of your image collections.

You need to `configure a service`_ with your custom loader and tag it with
Register it: automatically
^^^^^^^^^^^^^^^^^^^^^^^^^^

By default, your loader will be automatically registered as it implements the ``LoaderInterface``.

You will be able to reference and use your custom loader in your configuration by using its Fully Qualified Class Name.

Register it: manually
^^^^^^^^^^^^^^^^^^^^^

If you want to give it a different name you need to `configure a service`_ with your custom loader and tag it with
``liip_imagine.binary.loader``.

To register ``App\Service\MyCustomDataLoader`` with the name
Expand Down
14 changes: 12 additions & 2 deletions Resources/doc/filters.rst
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,17 @@ The ``LoaderInterface`` has the method ``load``, which is provided an instance
of ``ImageInterface`` and an array of options. It must return an
``ImageInterface``.

You need to `configure a service`_ and tag it ``liip_imagine.filter.loader``.
Register it: automatically
^^^^^^^^^^^^^^^^^^^^^^^^^^

By default, your filter will be automatically registered as it implements the ``LoaderInterface``.

You will be able to reference and use your custom filter when defining filter sets in your configuration by using its Fully Qualified Class Name.

Register it: manually
^^^^^^^^^^^^^^^^^^^^^

If you want to give it a different name you need to `configure a service`_ and tag it ``liip_imagine.filter.loader``.

To register a filter ``App\Service\MyCustomFilter`` as
``my_custom_filter``, use the following configuration:
Expand Down Expand Up @@ -124,7 +134,7 @@ to the image, by passing configuration as third parameter to ``applyFilter``:
public function filter(int $width, int $height) {
$filter = '...'; // Name of the `filter_set` in `config/packages/liip_imagine.yaml`
$path = '...'; // Path of the image, relative to `/public/`

if (!$this->cacheManager->isStored($path, $filter)) {
$binary = $this->dataManager->find($filter, $path);

Expand Down
12 changes: 11 additions & 1 deletion Resources/doc/post-processors.rst
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,17 @@ for your custom post-processor.
}
}

You need to `configure a service`_ with your custom post-processor and tag it
Register it: automatically
^^^^^^^^^^^^^^^^^^^^^^^^^^

By default, your post-processor will be automatically registered as it implements the ``PostProcessorInterface``.

You will be able to reference and use your custom post-processor in your configuration by using its Fully Qualified Class Name.

Register it: manually
^^^^^^^^^^^^^^^^^^^^^

If you want to give it a different name you need to `configure a service`_ with your custom post-processor and tag it
with ``liip_imagine.filter.post_processor``.

To register ``App\Service\MyCustomPostProcessor`` with the name
Expand Down
6 changes: 5 additions & 1 deletion Tests/LiipImagineBundleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,11 @@ public function testAddLoaders(): void
*/
protected function createContainerBuilderMock()
{
return $this->createObjectMock(ContainerBuilder::class, [], false);
return $this->createObjectMock(ContainerBuilder::class, [
'getExtension',
'addCompilerPass',
'registerForAutoconfiguration',
Copy link
Member

@dbu dbu Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in #1558 i kill the createObjectMock method. this might get "interesting" when merging 2.x to 3.x.

we could also make it a normal mock and expect the registerForAutoConfiguration method to be called and return either a mock definition or an empty real definition (in the test that actually uses this). then we can even expect $this->exactly(3) to be sure the passes are registered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not used to mock things in test. Can I have some guidance ? I'm not exactly sure how to fix the issue.

I could re-target 3.x but I think those changes are worth in 2.x.

], false);
}

/**
Expand Down
Loading