[PP-1] Support setting service parameters via environment variables

Just override the parameter from your own ServiceProvider.

Instead of using symfony magic and replace the way the container is being built, use the drupal/symfony way to alter services/parameters in the container:
https://www.drupal.org/docs/drupal-apis/services-and-dependency-injectio...

In essence what we did is to create file: my_module/src/MyModuleServiceProvider.php (the name matters) looks like this:

namespace Drupal\my_module;

use Drupal\Core\DependencyInjection\ContainerBuilder;
use Drupal\Core\DependencyInjection\ServiceProviderBase;

class MyModuleServiceProvider extends ServiceProviderBase {
  /**
   * {@inheritdoc}
   */
  public function alter(ContainerBuilder $container) {
    $cors_config = $container->getParameter('cors.config');
    $cors_config['allowedOrigins'] = ['https://' . $_ENV['CONSUMER_HOST']];
    $container->setParameter('cors.config', $cors_config);
  }
}

Make sure clear cache (so container will be compiled fresh) - drush cr / drush cc container

@bircher
I added some basic test coverage for resolving the env variable at compile time.

I'm also playing around with the idea to support using environment variables during runtime, see the --runtime branch.
This is clearly a bit more involved, and I think belongs really into its own issue in the first place.

@jkdev by doing it that way the values from environment variables are cached in the container, which means you need to rebuild caches anytime an environment variable is changed. I don't think we want that.

@duadua looks like you haven't pushed anything yet to the issue-3249970--runtime and 3249970-support-setting-service branches. I'm not sure why, but it contains some commits from other issues. Only issue-3249970 contains relevant commits.

• Created a new branch issue-3249970-11x, cherry-picked the commits from @duadua and brought it up to date with 11.x;
• Fixed PHPCS violations;
• Tested with setting an environment variable via ddev/config.yaml, verifying it is used after this patch is applied;

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

There is some problem with tests from `CKEditor5PluginManagerTest` class.
Functions `testInvalidPluginDefinitions` and `testDerivedPluginDefinitions` are not passed.

The error message from CI:

Failed asserting that exception of type "AssertionError" matches expected exception "Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException". Message was: "The file specified by the given app root, relative path and file name (vfs://root/sites/simpletest/17538585/core/modules/mysql/mysql.info.yml) do not exist."

I don't know how to solve this issue.

Haven't completely found out why, but the DrupalKernel changes lead to the addition of Symfony\Component\DependencyInjection\Compiler\ResolveEnvPlaceholdersPass, and that makes it necessary to re-resolve the `%container.modules%` parameter to ModuleHandler in the test as well.

diff --git a/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php b/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
index 30cd8fe3b60..23decc3ba8f 100644
--- a/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
+++ b/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
@@ -167,7 +167,9 @@ private function mockModuleInVfs(string $module_name, string $yaml, array $addit
     // The exception to the above elegance: re-resolve the '%app_root%' param.
     // @see \Symfony\Component\DependencyInjection\Compiler\ResolveParameterPlaceHoldersPass
     // @see \Drupal\Core\DrupalKernel::guessApplicationRoot()
-    $container->getDefinition('module_handler')->setArgument(0, '%app.root%');
+    $container->getDefinition('module_handler')
+      ->setArgument(0, '%app.root%')
+      ->setArgument(1, '%container.modules%');

     // To discover per-test case config schema YAML files, work around the
     // static file cache in \Drupal\Core\Extension\ExtensionDiscovery. There is

Thanks a lot. It solved the problem. However, there are still 2 tests that haven't been passed relating to attributes and annotations on simple deriver.
See if someone can solve these problems.

Indeed, it's strange to have to reinject all the parameters. But given that the test recreates a ContainerBuilder with a FrozenParameterBag and, on the other side the Drupal container is compiled with an EnvPlaceholderParameterBag, I guess that's the reason.
I don't know if the test is just an edge case or if there's a real impact on Drupal's behavior.

One comment: should document the additional services/parameters that need to be re-resolved, accounting for the additional compiler pass.

Also, as tagged, this issue needs a change record documenting how to use the environment variable placeholders.

I don't think it's necessary to add an other comment for additional services that need to be re-resolved. In that case, only the comment on the test is needed because it's really connected to this behavior.

Concerning the documentation, I would agree to add a comment but I don't know where to put it. Does the sites/default/default.services.yml file seem right to you (After that, the content of the documentation is the same as for Symfony)?
I don't think we have access to the technical documentation repository to add a section.

I don't think it's necessary to add an other comment for additional services that need to be re-resolved. In that case, only the comment on the test is needed because it's really connected to this behavior.

Sorry if this was unclear, but yes, this is the only comment that was needed. My previous post referred to my suggestion on the MR.

Concerning the documentation, I would agree to add a comment but I don't know where to put it. Does the sites/default/default.services.yml file seem right to you (After that, the content of the documentation is the same as for Symfony)?
I don't think we have access to the technical documentation repository to add a section.

The ask isn't for more documentation in code. You can follow these instructions on how to add a change record to a Drupal issue. For an actual example, look at the change record documenting autowired services being added to Drupal.

Okay, I create a change record to Drupal issue.

I'm getting a weird issue with an edge case. Reading the comments above, its seems to be related with the issue encountered in #20 #21 & #22

I have monolog installed, and in the parameters section of monolog.services.yml, have these entries (shortened):

parameters:
  syslog_format: '%%channel%%|%%datetime%%|[...]|%%message%%'
services:
    monolog.formatter.syslog_line:
      class: Monolog\Formatter\LineFormatter
      arguments: [ '%syslog_format%', 'U' ]
      shared: false

With the patch installed, rebuilding caches fails with the following error:
You have requested a non-existent parameter "channel". Did you mean this: "monolog.channel_handlers"?

Basically, %%channel%% is interpreted as %channel%, so it tries to resolve the string as a service parameter named channel.
If I then try to escape this sequence (so %%%%channel%%%%), the registration succeeds, but the parameter is registered as %%channel%%, while it should be stored as %channel%

A weird workaround I found to work is the following:

parameters:
  env(MONOLOG_SYSLOG_FORMAT): '%%%%channel%%%%|%%%%datetime%%%%|[...]|%%%%message%%%%'
services:
    monolog.formatter.syslog_line:
      class: Monolog\Formatter\LineFormatter
      arguments: [ '%env(string:MONOLOG_SYSLOG_FORMAT)%', 'U' ]
      shared: false

That way, registration succeeds, and the parameter is stored as expect, with a single %.

I have monolog installed, and in the parameters section of monolog.services.yml, have these entries (shortened):

parameters:
  syslog_format: '%%channel%%|%%datetime%%|[...]|%%message%%'
services:
    monolog.formatter.syslog_line:
      class: Monolog\Formatter\LineFormatter
      arguments: [ '%syslog_format%', 'U' ]
      shared: false

Can you tell us which version of the monolog module you're using (I can't find this parameter in the monolog module.)?

It's not related to monolog, it's just the mean through which I found the issue, it can be reproduced using core only (this project uses 10.3.14):

In a parameter_test_module.services.yml:

parameters:
  broken_parameter: '%%somevalue%%'
  
services:  
  parameter_test_module.example:
    class: Drupal\parameter_test_module\Example
    arguments: ['%broken_parameter%']

Upon clearing caches:

drush cr
In ParameterBag.php line 93:                                   
  You have requested a non-existent parameter "somevalue".

Without the patch, the parameter is properly registered in the container as broken_parameter: '%%somevalue%%'

I don't understand why you would use a parameter in this case. Why not pass the value directly.

services:  
  parameter_test_module.example:
    class: Drupal\parameter_test_module\Example
    arguments: ['%%somevalue%%']

After all, I don't know if your case is a problem.

Well, the why do not really matter: it's a supported behavior according to symfony documentation, which works without the patch, so I'd say it is a problem, since it's a regression.

https://symfony.com/doc/current/configuration.html#configuration-parameters

If some parameter value includes the % character, you need to escape it by adding another %, so Symfony doesn't consider it a reference to a parameter name

I suspect some kind of double resolution of parameters within a compiler pass (or rather, two passes); but I can't say which one, nor why it kicks twice

I suppose the part for configuration parameters with %% needs to be, at least commented. Or better refactored.

Perhaps we could also modify the yaml schema for services. Visual Studio Code doesn't like the !php/[...] part.

I think this change in DrupalKernel might be the wrong approach:

@@ -1396,7 +1396,7 @@ protected function compileContainer() {
     $container->setParameter('app.root', $this->getAppRoot());
     $container->setParameter('site.path', $this->getSitePath());

-    $container->compile();
+    $container->compile(TRUE);
     return $container;
   }

Looking at the documentation for \Symfony\Component\DependencyInjection\ContainerBuilder::compile(bool $resolveEnvPlaceholders = false):

     * @param bool $resolveEnvPlaceholders Whether %env()% parameters should be resolved using the current
     *                                     env vars or be replaced by uniquely identifiable placeholders.
     *                                     Set to "true" when you want to use the current ContainerBuilder
     *                                     directly, keep to "false" when the container is dumped instead.

Generally the Drupal container does get dumped (and cached), outside of kernel tests. So $resolveEnvPlaceholders should not be set to TRUE per that documentation. In addition, resolving the environment placeholders at compile time would mean that it does not work as Symfony documents:

Use the special syntax %env(ENV_VAR_NAME)% to reference environment variables. The values of these options are resolved at runtime (only once per request, to not impact performance) so you can change the application behavior without having to clear the cache.

Instead, with the placeholders being resolved at compile time, any changes to env variables after the container is cached would not update the container parameter values without a cache rebuild. (If it's fine to support environment variables this way for Drupal, then that difference would need to be documented in the change record.)

And also, as seen in #20 and #28, the extra compile pass resolving the environment variable placeholders is having unexpected side effects.

I think the necessary change to support environment variables is a larger lift: Drupal\Component\DependencyInjection\Container would need to reproduce the environment variable replacement functionality that Symfony\Component\DependencyInjection\Container has, but since the Drupal container is quite different from Symfony's, this effort doesn't look straightforward to me.

Separately, I think changing the Yaml parser to handle constants is out of scope for this issue. Work for that is being done in #2951046: Allow parsing and writing PHP class constants and enums in YAML files.

I guess you are right.

I also don't think that modifying the Drupal\Component\DependencyInjection\Container file is a good idea. It's well specified in the comments the reasons for not instantiating methods like getParameterBag(). Also, why uppercase parameters are not supported (e.g. env(KEY): 'value').

 * This container is different in behavior from the default Symfony container in
 * the following ways:
 *
 * - It only allows lowercase service and parameter names, though it does only
 *   enforce it via assertions for performance reasons.
 * - The following functions, that are not part of the interface, are explicitly
 *   not supported: getParameterBag(), isFrozen(), compile(),
 *   getAServiceWithAnIdByCamelCase().

Maybe @Jkdev approach #7 is the closest to Drupal way.

Here's a first idea (beta):

$settings['my_custom_value'] = $_SERVER['ENV_CUSTOM_VALUE'] ?? ''; // foobar

use Drupal\Core\DependencyInjection\ContainerBuilder;
use Drupal\Core\DependencyInjection\ServiceProviderBase;
use Drupal\Core\Site\Settings;

final class SettingsServiceProvider extends ServiceProviderBase
{
  public function alter(ContainerBuilder $container): void
  {
    $settings = Settings::getAll();
    $this->addSettingsParameters($settings, $container);
  }

  private function addSettingsParameters(array $settings, ContainerBuilder $container, string $prefix = 'settings'): void
  {
    foreach ($settings as $key => $value) {
      $parameterName = $prefix . '.' . $key;

      if (is_array($value)) {
        $container->setParameter($parameterName, $value);
        $this->addSettingsParameters($value, $container, $parameterName);
      }
      else {
        $container->setParameter($parameterName, $value);
      }
    }
  }
}

services:
  example.service:
    class: Drupal\example\Service
    arguments:
      - '%settings.my_custom_value%'

final class Service
{
  public function __construct(
    // foobar
    #[\SensitiveParameter]
    public string $value
  ) {
  }
}

Environmental parameters need to be resolved at runtime - having them only take affect on cache rebuild would be a really odd behaviour divergence from Symfony. This means we have to change our approach on this issue and store enviroment parameters separately from the other parameters so we can resolve them in \Drupal\Component\DependencyInjection\Container::getParameter(). Shouldn't be too bad.

Ok I've got this working, assisted by AI. This is all quite messy because we're not using Symfony's container dumping which handles environment based parameters much more elegantly - the gets from the environment are written to the dumped PHP. Therefore we perhaps should block this on #3583505: Use Symfony PhpDumper instead of a serialized array container structure which would mean we'd have to do less work.

Blocking on #3583505: Use Symfony PhpDumper instead of a serialized array container structure per #41.