Skip to content

[DI] Preserve placeholder exception for dynamic arrays #29270

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

Closed
wants to merge 1 commit into from
Closed

[DI] Preserve placeholder exception for dynamic arrays #29270

wants to merge 1 commit into from

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Nov 21, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...

Here's some initial work to deal with envs for array nodes in config.

From #27683 (comment) this would be a first step.

What's still needed is a way to know about the types, e.g. a env resolving to string should be preserved as array($v) to be really flexible.

cc @stof

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 22, 2018

Thinking a bit more about it.. one can only expect env placeholders for string values. Maybe having specialized builder API is the better approach, think;

->ifEnv()
->ifNotEnv()
->ifArrayEnv()
->ifNotArrayEnv() // we need this one for e.g. trusted_hosts instead of isString()
->then()

@mtamazlicaru
Copy link

mtamazlicaru commented Nov 22, 2018

@ro0NL for me seems ok.

@nicolas-grekas nicolas-grekas added this to the next milestone Nov 22, 2018
@MrMitch
Copy link

MrMitch commented Feb 26, 2019

@stof @ro0NL Hi! Do you need any assitance on this ?
I'm currently facing the exact same issue as mentionned in #28137 (i.e. being able to configure the delivery_addresses of swiftmailer from an env var of varying size between each env).

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
@@ -265,6 +265,21 @@ public function testDiscardedEnvInConfig(): void
$this->assertSame('1', $container->getParameter('boolish'));
}

/**
* @expectedException \Symfony\Component\Config\Definition\Exception\InvalidTypeException
* @expectedExceptionMessage A dynamic value is not compatible with a "Symfony\Component\Config\Definition\PrototypedArrayNode" node type at path "env_extension.nodes".
Copy link
Member

Choose a reason for hiding this comment

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

should use $this->expectException* now

@nicolas-grekas nicolas-grekas modified the milestones: 4.4, next Nov 5, 2019
@fabpot
Copy link
Member

fabpot commented Aug 11, 2020

@ro0NL What's the status here?

@ro0NL
Copy link
Contributor Author

ro0NL commented Aug 12, 2020

@fabpot it's kinda on hold, but we can discuss further in #27683. Closing as such.

What i particularly dont like about this implementation is, each affected node having to handle the case themselves, i.e.;

->beforeNormalization()
    ->ifString()
    ->then(function ($v, bool $isEnv) {
        return $isEnv ? $v : array($v);
    })
->end()

It hurts maintenance. I prefer something generic that just works, but i dont know how to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants