LiipImagineBundle icon indicating copy to clipboard operation
LiipImagineBundle copied to clipboard

Array of "default" loader (bug)

Open tebaly opened this issue 2 years ago • 5 comments

Preconditions

debug:conf liip_imagine

Current configuration for extension with alias "liip_imagine"
=============================================================

liip_imagine:
    resolvers:
        ...
    loaders:
        default:
            filesystem: # from bundle
                data_root:
                    - /some/path
                locator: filesystem
                allow_unresolvable_data_roots: false
                bundle_resources:
                    enabled: false
                    access_control_type: blacklist
                    access_control_list: {  }
            flysystem: # from root app
                filesystem_service: oneup_flysystem.default_filesystem_filesystem

Steps to reproduce

  1. Define default loader in bundle
  2. Define default loader in root app

Expected result

  1. default loader config will be override

Actual result

  1. array of "default" loaders (bug)

tebaly avatar Mar 09 '22 11:03 tebaly

hi, this is the expected behaviour. this configures the default loaders which will be a list of loaders that is tried unless a different data_loader is specified on a filter set.

i think if you define the loaders section twice, it is up to symfony configuration management how it handles that. i think the concept is to merge, so what you see is expected.

dbu avatar Mar 14 '22 12:03 dbu

The bundle developer sets the configuration and its processing. In this case, an ARRAY is used, which should not be there. Because the default is ONE. This is a bug, obviously.

tebaly avatar Mar 16 '22 19:03 tebaly

the class processing the configuration is https://github.com/liip/LiipImagineBundle/blob/2.x/DependencyInjection/LiipImagineExtension.php

looking at it, i don't see what would be wrong, but if you can do a pull request with a functional test that shows the problem, i can look into it.

dbu avatar Mar 17 '22 07:03 dbu

Of course dear friend. After all, the error is in the configuration itself.

https://github.com/liip/LiipImagineBundle/blob/1a04fac4dd9d318a3a42fedc81896d874b8a7e64/DependencyInjection/Configuration.php#L82

https://github.com/liip/LiipImagineBundle/blob/1a04fac4dd9d318a3a42fedc81896d874b8a7e64/DependencyInjection/Configuration.php#L86

Nobody checks if there is one default resolver or two. Nobody cares.

tebaly avatar Mar 17 '22 11:03 tebaly

the configuration class processes one configuration at a time afaik. this leads to a hashmap with the configuration structure, which is then merged by the symfony configuration handling before giving it to the LiipImagineExtension

the two lines make sure that the default key in the loaders section exists. the merging is done by symfony. and it is perfectly fine to have multiple loaders. the chain loader is then trying each loader and picking the first result.

dbu avatar Mar 17 '22 13:03 dbu