Use container parameters instead of settings

I asked myself multiple time why we don't use container parameters. Some possible answers could be: you need to rebuild the container to get new data in there (at least by default),
we just didn't knew about that etc.

This basically means that all Settings we do have are things which are needed before the container is booted up, and so it is a limited amount of usecases we potentially could
all add new methods onto Settings.

The way Settings *started* was for things that we need *before* we have a container, like the APC classloader setting.

This makes sense to me, and seems like a good use of the tools already at our disposal. Caveat is the need to rebuild the container any time you change these values. We have rebuild.php right now, but to make that really easy we ought to add a CLI version too. That would, for me, adequately address the DX question.

+++ b/core/lib/Drupal/Core/CoreServiceProvider.php
@@ -42,11 +42,12 @@ class CoreServiceProvider implements ServiceProviderInterface  {
+    $container->setParameter('app.root', DRUPAL_ROOT);
+

I especially love this!

Added a couple of bits:

• Ensured that the keyvalue setting is used in the kernelTestBase, so there are much less failures
• Adapted the code in install.core.inc to write a install.php/services.yml file, wanted to do that anyway for backend overrides

Some progress ...

This fixes some of the failures. The remaining ones are caused by the missing functionality of being able to inject parameters into the kernel container rebuild.

The YamlFileLoader had internal caching which caused issues as cache is per file, so changing the services.yml file doesn't end up in the container.

Remove the assignment so it is clear that this kinda ready for review.

Rerolled.

This could be it.

Shouldn't this patch also remove the twig debug/autoreload/cache/… settings from example.settings.local.php?

Other than that, I only could find one nitpick. I looked at everything except the installer.

+++ b/core/INSTALL.txt
@@ -147,26 +147,29 @@ INSTALLATION
+      Drupal will try to automatically create a settings.php/servces.yml configuration file,

80 cols.

I couldn't find anything other than #20 to object to, either. Thanks, dawehner!

Shouldn't this patch also remove the twig debug/autoreload/cache/… settings from example.settings.local.php?

Good catch. I decided to copy over the documentation of the settings.php example into the yml file.

80 cols.

It is so great that we have this standards, now the patch just gets much "easier" to review.

Tagging.

Fixed a typo and more 80 cols nitpicks.

Combined with #21, I think this is RTBC.

Oops! A lot of unrelated stuff ended up in that patch.

This needs a change record, but it needs to update an existing change record. But https://www.drupal.org/node/1922666 is already published and we'd need to create a draft revision of the changes, which d.o doesn't support. So I created a new draft change notice that contains the changes over at https://www.drupal.org/node/2317817. We can then copy that to the existing change record once this is committed.

We need a change record that announces that the installer is going to create a services.yml file - or fail if it can not create one.

Done: https://www.drupal.org/node/2317985.

+++ b/core/INSTALL.txt
@@ -147,26 +147,30 @@ INSTALLATION
+      file sites/default/default.settings.php/default.services.yml as a
...
+      default.settings.php/default.services.yml file with the command:

This doesn't look right. I think you're using "/" to mean "or" but we're talking about file paths in this context and / has a very specific meaning.

what about that?

files should be plural. (Do not credit me.)

This seems actually rather major tbh.

Perfect 3way merge.

not my day.

it is green

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1022,6 +1025,26 @@ protected function writeSettings(array $settings) {
+  protected function containerParameterSet($name, $value) {
+    $filename = $this->siteDirectory . '/services.yml';
+    chmod($filename, 0666);
+
+    $services = Yaml::decode(file_get_contents($filename));
+    $services['parameters'][$name] = $value;
+    file_put_contents($filename, Yaml::encode($services));
+
+    // Clear the YML file cache.
+    YamlFileLoader::reset();
+  }

how much cleaner is this than our writing of php in settings.php in the installer.

love it.

next step down this path - db connection settings...

db connection settings is a litte bit difficult due to fast page caching.

Are we OK with all of this backend config being exposed to the public? Cache backends, bins, etc.? As far as I can tell .yml files are served up by our default .htaccess rules. I suggest we block that for whole Drupal site. If I am off base here, please reset to RTBC.

Are we OK with all of this backend config being exposed to the public? Cache backends, bins, etc.? As far as I can tell .yml files are served up by our default .htaccess rules. I suggest we block that for whole Drupal site. If I am off base here, please reset to RTBC.

We should absolutely block them but well, services.yml already exists, so opened a follow up #2321487: Ensure to not serve yml files inside Drupal

@crell agreed

Some good points. I have made changes for all of those points, except the shortening of the comments.

Somehow removed the default settings from the main patch.

Another missing file, sorry for noise!

+++ b/sites/default/default.services.yml
@@ -0,0 +1,53 @@
+    # Twig debugging:
+    #
+    # When debugging is enabled:
+    # - The markup of each Twig template is surrounded by HTML comments that
+    #   contain theming information, such as template file name suggestions.
+    # - Note that this debugging markup will cause automated tests that directly
+    #  check rendered HTML to fail. When running automated tests, 'twig_debug'
+    #   should be set to FALSE.
+    # - The dump() function can be used in Twig templates to output information
+    #   about template variables.
+    # - Twig templates are automatically recompiled whenever the source code
+    #  changes (see twig_auto_reload below).
+    #
+    # For more information about debugging Twig templates, see
+    # http://drupal.org/node/1906392.
+    #
+    # Not recommended in production environments
+    # @default false

I would also agree with @sun that comments like this seem a little on the verbose side for a yaml file.

@sun
Thank you for the review, good points, indeed!

Unlike PHP in settings.php, excessively lengthy comments do negatively affect the YAML parser performance. (Next to harming DX.)

In the first patch, I had heavily shortened these comments already. Was there a particular reason for reverting that?

I don't think we should repeat the epic misery of settings.php comment novels. Instead, let's have minimum info inline + provide more help elsewhere — either in a separate file, or directly pointing to online documentation.

While I do agree in principal dropping that information without a proper replacement yet, is a bad idea. A follow up could move this documentation to drupal.org or somewhere else in the code #2322539: Improve (and shorten) documentation in default.services.yml.

So back to RTBC

muh.

1. +++ b/core/includes/install.core.inc
   @@ -1886,128 +1886,168 @@ function install_check_requirements($install_state) {
   +      // If settings.php does not exist, throw an error.
   ...
   +        // If settings.php is not readable, throw an error.
   ...
   +        // If settings.php is not writable, throw an error.
   

   If settings.php or services.yml...

2. +++ b/core/modules/system/src/Tests/KeyValueStore/DatabaseStorageExpirableTest.php
   @@ -18,8 +19,9 @@ class DatabaseStorageExpirableTest extends StorageTestBase {
   -    module_load_install('system');
   +    include_once DRUPAL_ROOT . '/core/modules/system/system.install';
   
   +++ b/core/modules/system/src/Tests/KeyValueStore/DatabaseStorageTest.php
   @@ -18,7 +19,7 @@ class DatabaseStorageTest extends StorageTestBase {
   -    module_load_install('system');
   +    include_once DRUPAL_ROOT . '/core/modules/system/system.install';
   

   How come?

How come?

Typical out of scope change.

let's just re-RTBC

It was green again.

Sorry for nitpicking a comment apart but this still does not feel right.

+++ b/core/INSTALL.txt
@@ -147,26 +147,30 @@ INSTALLATION
+      Drupal will try to automatically create a settings.php/services.yml
+      configuration file, which is normally in the directory sites/default (to
+      avoid problems when upgrading, Drupal is not packaged with this file). If
+      auto-creation fails, you will need to create this file yourself, using the
+      files sites/default/default.settings.php and
+      sites/default/default.services.yml as a template.

Should this not be something like: Drupal will try to automatically create settings.php and services.yml files, which are normally in the directory sites/default (to avoid problems when upgrading, Drupal is not packaged with this file). If auto-creation of either file fails, you will need to create the file yourself. Use the template sites/default/default.settings.php or sites/default/default.services.yml respectively.

We could avoid most of the mumbo-jumbo in install.core.inc by maintaining the core-supplied default file in /core/default.services.yml.

Well, then you are back in having to merge the values when building the container right? Feel free to open a follow up.

Sorry for nitpicking a comment apart but this still does not feel right.

You can't be worse than the bot :P Just copy and pasted your solution. It sounded better to read.

@sun
Well your approach would certainly be nice as you would no longer have to figure out whether a default file exists etc. though you would have to either special case the logic
for the services.yml file, which is a bad idea, or move the default settings file as well, which feels like a good idea tbh. as long we have a good way to discover it.

I guess we can move this back to RTBC now ?

here is the lovely exception you get

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "logger.factory" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 873 of /Volumes/devdisk/dev/sites/drupal8alt.dev/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('logger.factory')
Symfony\Component\DependencyInjection\ContainerBuilder->get('logger.factory', 1)
Drupal\Core\DependencyInjection\ContainerBuilder->get('logger.factory')
Drupal::logger('php')
_drupal_log_error(Array, )
_drupal_error_handler_real(2, 'file_get_contents(sites/default/services.yml): failed to open stream: Permission denied', '/Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php', 257, Array)
_drupal_error_handler(2, 'file_get_contents(sites/default/services.yml): failed to open stream: Permission denied', '/Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php', 257, Array)
file_get_contents('sites/default/services.yml')
Drupal\Core\DependencyInjection\YamlFileLoader->loadFile('sites/default/services.yml')
Drupal\Core\DependencyInjection\YamlFileLoader->load('sites/default/services.yml')
Drupal\Core\DrupalKernel->compileContainer()
Drupal\Core\DrupalKernel->initializeContainer(1)
Drupal\Core\Installer\InstallerKernel->initializeContainer()
Drupal\Core\DrupalKernel->boot()
install_begin_request(Array)
install_drupal()

Patch attached tidies up some stuff and add important checking to system_requirements().

I have not fixed the exception - you get it if you have a non writeable empty services.yml in place.

@alexpott
When exactly does that happen for you?
The patch does the exact some for the services file as for the settings.php file. Do you have some special way to run your installation?

here is the lovely exception you get

Well, the actual reason for that crazyness already exists without that patch: #2325385: Ensure that the error handler does not throw exception itself.

To recreate the exception:

• sites/default is not writable by the webserver
• sites/default/settings.php exists but is not writable by the webserver
• sites/default/services.yml exists but is not writabel by the webserver

This should result in a requirement error.

#2325385: Ensure that the error handler does not throw exception itself. might have resolved this

Wow, showing permissions feels almost like getting naked.

According to your description I tried to setup the directory and files:

├── [drwxr-xr-x dawehner dawehner]  default
│   ├── [-rw-rw-r-- dawehner dawehner]  default.services.yml
│   ├── [-rw-rw-r-- www-data www-data]  default.settings.php
│   ├── [drwxrwxrwx dawehner dawehner]  files
│   ├── [-rwxr-xr-x dawehner dawehner]  services.yml
│   ├── [-rw-r--r-- dawehner dawehner]  settings.local.php
│   └── [-rwxr-xr-x dawehner dawehner]  settings.php

It tells me that both services.yml and settings.php is not writable, as expected.

Sadly the code never wents into _drupal_log_error().

Looking bad at your exception all I needed to reproduce the issue is to drop the readable flag from the services file.
Now we just have to ensure that the kernel doesn't load a non-readable file. This results in a proper settings page, see screenshot.

Back to RTBC.

I was able to do a clean install with no issues, apart from #2306271: drupal_set_message(t('whatever')) is double escaped. Didn't spot any code issues in my review.

The DX of this looks like a big win. I've always hated the $settings vs. $config separation, because site builders shouldn't be expected to understand this level of sausage factory. I understand this patch doesn't get us all the way there, but it's a good step in the right direction. The docs improvements are welcome, as well.

One concern that was raised in IRC by beejeebus is the fact that rebuilding the container is an expensive operation, and particularly gnarly on a multi-web head environment with a shared filesystem. Originally it was apparently discussed to only do this during module install/uninstall to keep this pain down, but this will make it so we may do it at random times. ATM this doesn't feel like a commit-blocker, because the settings moved to container parameters in this patch are those you'd only want to change on a dev environment, where this would be less of an issue, generally. But something to keep in mind for future patches we try and use this pattern elsewhere.

In the meantime, committed and pushed to 8.x. Thanks!

I just updated https://www.drupal.org/node/1922666, not sure if we want to update the timestamp on that one. Made a minor correction for the twig_debug default. I think someone with privileges can delete https://www.drupal.org/node/2317817 now.

And I published https://www.drupal.org/node/2317985 after making one minor tweak. Thanks @Wim Leers for writing both of those up!