1up-lab / OneupFlysystemBundle

A Flysystem integration for your Symfony projects.
MIT License
634 stars 118 forks source link

Cannot use ENV var processors in the adapter configuration #281

Closed yakobe closed 1 year ago

yakobe commented 1 year ago

Bug Report

Q A
BC Break yes
Version 4.8.0

Summary

Before the change in https://github.com/1up-lab/OneupFlysystemBundle/pull/277 it was possible to resolve environment variables in the configuration. E.g.

oneup_flysystem:
  adapters:
    local_adapter:
      local:
        location: '%env(resolve:APP_DATA_DIR)%'

And the APP_DATA_DIR could then be set to%kernel.project_dir%/var/data

However, now is is not possible and yields the error:

!!  In MergeExtensionConfigurationPass.php line 200:
!!
!!    Using a cast in "env(resolve:APP_DATA_DIR)" is incompatible with resolution
!!     at compile time in "Oneup\FlysystemBundle\DependencyInjection\OneupFlysyst
!!    emExtension". The logic in the extension should be moved to a compiler pass
!!    , or an env parameter with no cast should be used instead.

I guess it is not possble to use the environment variable processors now because the configuration needs to be compiled first so that the adapter itself can be changed with an environment variable.

How to reproduce

Configure a local adapter and set it's location to location: '%env(resolve:APP_DATA_DIR)%'. Then add %kernel.project_dir%/var/data to your .env.local file

yakobe commented 1 year ago

I could probably work around it by not using environment variables to set the location directory, however it was useful to be able to change the location relative to the project directory (or sometimes for functional test even to use the cache directory with %kernel.cache_dir%/data/.

If the feature in #277 is more important then it's not critcal, just thought i would raise it as a BC break in case there is a way to continue using the processors somehow. 😄

bytehead commented 1 year ago

Thanks for reporting! I get your point, need to think about it. As #277 is the only new thing in 4.8.0, I recommend to use 4.7.0 until we have a solution here.

alexandre-le-borgne commented 1 year ago

(translated)

After a bit of reading about the obscure workings of Symfony's dependency injection component, I'm beginning to think that my PR may not be the right solution according to Nicolas Grekas & co. However, they don't seem to offer an official solution for this case...

I could have used a parameters.local.yaml, ignored by git and imported into services.yaml; allowing me to override the configuration without the environment variables. A solution that would also concern this issue, as the "location" is not something you want to change at runtime.

yakobe commented 1 year ago

We do something a bit similar to that to switch adapters in prod/test/dev etc.

We have a parameter for the service name:

parameters:
  filesystem_service: 'oneup_flysystem.app_data_%env(DATA_STORE)%_filesystem'

and then we pass that service to a wrapper that manages our file system interactions

services:
  App\FileSystem\FilesystemOperator:
    arguments:
      $filesystem: '@=service(parameter(''filesystem_service''))'

Not perfect, but it does the job

wiistriker commented 1 year ago

Purpose of the Flysystem library to provide abstraction layer between various kinds of filesystems, so in my opinion each adapter should be configured via dsn.

oneup_flysystem:
    adapters:
        data: %env(resolve:DATA_STORE_DSN)%

and in .env file we will have: DATA_STORE_DSN=local://%kernel.project_dir%/data

and in .env.local for example: DATA_STORE_DSN=sftp://user:password@host:port/root

In current configuration we cant have such behaviour.

bytehead commented 1 year ago

I'm afraid we'll have to revert the changes from #277 for the moment now.

MarieMinasyan commented 1 year ago

Hi,

Since the latest changes a simple configuration like this doesn't work anymore because the environment variables are resolved on build and not on runtime as they're supposed to be.

oneup_flysystem:
    adapters:
        attachments:
            awss3v3:
                client: app.storage.aws_s3_client
                bucket: "%env(AWS_S3_BUCKET)%"
                prefix: "app/attachments"
bytehead commented 1 year ago

See https://github.com/1up-lab/OneupFlysystemBundle/issues/281#issuecomment-1589217876

bytehead commented 1 year ago

I've reverted the failing changes in #285 and released 4.9.0.