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

Bundle doesn't support port configuration by environment variable #466

Open
zluiten opened this issue Jun 19, 2018 · 17 comments
Open

Bundle doesn't support port configuration by environment variable #466

zluiten opened this issue Jun 19, 2018 · 17 comments
Labels
Projects

Comments

@zluiten
Copy link

zluiten commented Jun 19, 2018

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 pushed a commit to zluiten/DoctrineMongoDBBundle that referenced this issue Jun 19, 2018
…alarNode to accept placeholder environment variables
@stof
Copy link
Member

stof commented Jun 19, 2018

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

@zluiten
Copy link
Author

zluiten commented Jun 19, 2018

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

@alcaeus
Copy link
Member

alcaeus commented Jun 19, 2018

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?

@jmikola
Copy link
Member

jmikola commented Jul 6, 2018

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 symfony/symfony#23888.

@stale
Copy link

stale bot commented Jan 21, 2019

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 stale bot added the Stale Issues that have not seen any activity in 30 days label Jan 21, 2019
@alcaeus
Copy link
Member

alcaeus commented Jan 21, 2019

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

@stale stale bot removed the Stale Issues that have not seen any activity in 30 days label Jan 21, 2019
@xabbuh
Copy link
Member

xabbuh commented Jan 21, 2019

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

@alcaeus
Copy link
Member

alcaeus commented Jan 22, 2019

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...

@zluiten
Copy link
Author

zluiten commented Jan 22, 2019

Yep, @symfony should backport symfony/symfony#23888 to 3.4

@alcaeus
Copy link
Member

alcaeus commented Jan 22, 2019

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

@zluiten
Copy link
Author

zluiten commented Jan 22, 2019

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

@xabbuh
Copy link
Member

xabbuh commented Jan 22, 2019

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',
            ],
        ],
    ],
];

@alcaeus
Copy link
Member

alcaeus commented Jan 22, 2019

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

@alcaeus
Copy link
Member

alcaeus commented Feb 12, 2019

@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?

@piszczek
Copy link

piszczek commented Mar 6, 2019

Hi, is there any progress in that?

@alcaeus
Copy link
Member

alcaeus commented Mar 6, 2019

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

@stale
Copy link

stale bot commented Apr 5, 2019

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 stale bot added the Stale Issues that have not seen any activity in 30 days label Apr 5, 2019
@alcaeus alcaeus added bug and removed Stale Issues that have not seen any activity in 30 days labels Apr 5, 2019
@alcaeus alcaeus added this to 4.x in Roadmap Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

6 participants