DoctrineMongoDBBundle icon indicating copy to clipboard operation
DoctrineMongoDBBundle copied to clipboard

Bundle doesn't support port configuration by environment variable

Open zluiten opened this issue 6 years ago • 17 comments

When configuring the metadata_cache_driver section to use environment variables for the port configuration

doctrine_mongodb:
    document_managers:
        default:
            metadata_cache_driver:
                type: memcached
                class: Doctrine\Common\Cache\MemcachedCache
                instance_class: Memcached
                host: '%env(DOCTRINE_MONGODB_CACHE_HOST)%'
                port: '%env(int:DOCTRINE_MONGODB_CACHE_PORT)%'

It'll fail building the container with the exception

In IntegerNode.php line 29:

  Invalid type for path "doctrine_mongodb.document_managers.default.metadata_cache_driver.port". Expected int, but got string.

This makes sense because the port configuration should be an integer as it is configured as an IntegerNode. This is therefore not going to work because the placeholder value is being a string.

I would propose to change the port node to be of type ScalarNode, that would also be consistent with the DBAL configuration section which accepts the port to be scalar as well which you can check here: https://github.com/doctrine/DoctrineBundle/blob/master/DependencyInjection/Configuration.php#L192

zluiten avatar Jun 19 '18 09:06 zluiten

Which version of Symfony are you using ? This should be solved by Symfony 4.1 without having to remove the validation for all others

stof avatar Jun 19 '18 09:06 stof

Ah, yes, we're still on the (latest) 3.4... I guess back porting the fix would solve the issue here. Not sure if that can be done while keeping BC

zluiten avatar Jun 19 '18 10:06 zluiten

TBF, I would prefer keeping it as an integerNode - a port is a number by definition; changing it to a scalarNode seems like an ugly workaround. What would be your suggestion @stof?

alcaeus avatar Jun 19 '18 15:06 alcaeus

I concur that this looks odd. I would expect %env()% to be evaluated before the configuration is validated; otherwise, there isn't much point to type validation.

The relevant change in Symfony 4.1 appears to be https://github.com/symfony/symfony/pull/23888.

jmikola avatar Jul 06 '18 14:07 jmikola

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jan 21 '19 07:01 stale[bot]

Can someone from @doctrine/team-symfony-integration shed some light on this? I'd expect %env()% to be resolved before types are checked 🤔

alcaeus avatar Jan 21 '19 15:01 alcaeus

Does it work with Symfony 4.1 (I guess this requires symfony/symfony#23888).

xabbuh avatar Jan 21 '19 18:01 xabbuh

Just tested the following config with Symfony 4.2, with no luck:

$config = [
    'document_managers' => [
        'dm1' => [
            'metadata_cache_driver' => [
                'type'           => 'memcached',
                'class'          => 'fooClass',
                'host'           => 'host_val',
                'port'           => '%env(MEMCACHE_PORT)%',
                'instance_class' => 'instance_val',
            ],
        ],
    ],
];

A look at DoctrineBundle shows that they also use scalarNode for ports, but it seems like a dirty workaround...

alcaeus avatar Jan 22 '19 06:01 alcaeus

Yep, @Symfony should backport https://github.com/symfony/symfony/pull/23888 to 3.4

zluiten avatar Jan 22 '19 07:01 zluiten

And what good does that do when it doesn't work in 4.2 either? 🤔

alcaeus avatar Jan 22 '19 08:01 alcaeus

@alcaeus Ah, but I think it should work with '%env(int:MEMCACHE_PORT)%', when specifying "type casting"

zluiten avatar Jan 22 '19 08:01 zluiten

Yes, the int part is important here. The following should work:

$config = [
    'document_managers' => [
        'dm1' => [
            'metadata_cache_driver' => [
                'type'           => 'memcached',
                'class'          => 'fooClass',
                'host'           => 'host_val',
                'port'           => '%env(int:MEMCACHE_PORT)%',
                'instance_class' => 'instance_val',
            ],
        ],
    ],
];

xabbuh avatar Jan 22 '19 08:01 xabbuh

I'll give that a shot and create a PR with a test. Thanks for your help so far

alcaeus avatar Jan 22 '19 08:01 alcaeus

@xabbuh I finally managed to give it another shot. If I do the above, the port config has the following value in the extension: env_79eccd8f6110d31f_int_MEMCACHE_PORT_14eebdfdaafea8526666484336b4ef06. This is then passed to the addServer call in the DoctrineBridge. Since the value contains no % signs, the value is given as is, with the Memcached extension rightfully complaining about the port not being an integer. I'm not very familiar with how env resolution is done when given in the config, do you happen to know what's going on here?

alcaeus avatar Feb 12 '19 06:02 alcaeus

Hi, is there any progress in that?

piszczek avatar Mar 06 '19 09:03 piszczek

Just one attempt to get it to work in a test in #532.

alcaeus avatar Mar 06 '19 11:03 alcaeus

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Apr 05 '19 12:04 stale[bot]