Re-export all built-in configuration in core

Does this include whether sites/default/settings.php should be in YML format?

@iantresman no it's only for existing default YAML configuration. The last time we did it, quite a lot had diverged subtly from how it would have been created new in core, so this is a case of re-exporting everything so it matches. There's an issue somewhere about moving more things to container parameters out of settings, although can't find it at the moment.

Re-titling so try to make the scope clearer.

Just a quick question for reviews later ... is this issue just about default configuration or also configuration shipped with test modules?

+1 for all the configuration of all modules test or otherwise.
I think we should add a console command in follow up to export that automatically so that contrib can also use that.

We have a module Configuration Refresh based on @alexpott's script (https://gist.github.com/alexpott/f0650fbb7954df8497e4), but it might need to be updated with current head :(

Well, to kick things off: #2567505: Get it working again.

I'll give it a try for a while.

I'll give it a try for a while.

Pretty neat shell line to enable all modules: ls | xargs drush -y en
Using #2567505: Get it working again. I could produce the following changes:

Should config in the installation folder contain a uuid?

This adds some proper test coverage and changes back some of the changes which got caused by the fact that language module was enabled on those exports.

Should config in the installation folder contain a uuid?

You talk about the examples in image.*, right? Good question.
Well, its what HEAD already does. I think its kinda similar to what panels does in D7

#13: looks like historically, it was a YES(#1969800: Add UUIDs to default configuration) and then NO(#2110615: Do not ship with default UUIDs) and then YES for part of it(#2110615-65: Do not ship with default UUIDs) and image styles are one of them.

This should at least fix the failure in CommentFieldNameTest. The changes are in interdiff.

+++ b/core/modules/user/config/install/user.role.anonymous.yml
@@ -1,7 +1,13 @@
+  - 'use text format restricted_html'
+  - 'access comments'
+  - 'access site-wide contact form'

+++ b/core/modules/user/config/install/user.role.authenticated.yml
@@ -1,7 +1,16 @@
+  - 'access comments'
+  - 'post comments'
+  - 'skip comment approval'
+  - 'access site-wide contact form'
+  - 'access shortcuts'

The problem is here. These permissions are actually given by the standard profile but now we are assigning them in the user module itself. This doesn't seem right. For example, minimal profile does not enable the contact module but these permissions would be assigned anyway. The problem was that the testing profile didn't give this permission earlier and the test passed. Now, user module gives this permission and the test fails.

Of course, this takes us to the question of if we can use this approach to solve this problem. I am guessing that the site's config export may not match the YAML in modules itself as the profiles will almost definitely change settings. I am of an opinion that differences such as these are okay.

The patch in #19 should also fix problems in shortcut tests - same explanation as above.

The interdiff in https://www.drupal.org/files/issues/interdiff-15-19_1.txt absolutely makes sense!
Well, we have to remove all permissions, so it actually matches the previous default.

On top of that I skipped the simpletest settings, not sure what is going on there yet.
We needed another fix, as system and user did not installed their default config, because they have been installed in the test earlier already.
On top of that the list of modules to test are now ordered alphabetically.

Good catch!
While reading the diff again I also realized that the path module sneaks in entries for the taxonomy config.

Added a followup: #2568077: Re-export all test configuration

Just putting to needs work, as we need to care about installation profiles, but for sure, reviews would be nice!

1. +++ b/core/modules/aggregator/config/install/core.entity_view_display.aggregator_feed.aggregator_feed.default.yml
   @@ -1,31 +1,32 @@
   +langcode: en
   +status: true
   +dependencies:
   
   +++ b/core/modules/aggregator/config/install/core.entity_view_display.aggregator_feed.aggregator_feed.summary.yml
   @@ -1,22 +1,22 @@
   +langcode: en
   +status: true
   +dependencies:
   

   Consistent ordering!!!!

   <3 <3

   The nitpicker in me rejoices!

2. +++ b/core/modules/book/config/install/field.field.node.book.body.yml
   @@ -18,5 +18,4 @@ default_value: {  }
   -third_party_settings: {  }
   

   Why are these being removed? Not saying it's bad, I would just like to understand.

3. +++ b/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php
   @@ -0,0 +1,214 @@
   + * Tests that the installed config matches the default config.
   

   <3

   Fantastic!

4. +++ b/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php
   @@ -0,0 +1,214 @@
   +  public function providerTestModuleConfig() {
   

   Shouldn't we create this list automatically? Or would a missing or extraneous module trigger fails anyway?

Shouldn't we create this list automatically? Or would a missing or extraneous module trigger fails anyway?

I guess we could just scan the files available in core/modules and be done with it? I was just too lazy to be lazy.

Why are these being removed? Not saying it's bad, I would just like to understand.

It somehow happens when you save an entity without any third part settings. I'm not sure either where it actually happens.

Some intermediate work dealing with the install profile. Currently the test installs the config from system and then installs standard profile, but standard profile seems not to override
config from system yet.

Just

Shouldn't we create this list automatically? Or would a missing or extraneous module trigger fails anyway?

for now.

After talking with alex we agreed that the install profile one should be a web test, to be 100% sure.

We have now some proper test coverage, let's get the review started.

* Fixed the failures
* Exported the minimal files as well
* Added a test for the minimal install profile

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -523,7 +523,7 @@ protected function validateDependencies($config_name, array $data, array $enable
              if (!empty($missing)) {
   -            return FALSE;
   +           return FALSE;
              }
   

   Incorrect and out of scope change

2. +++ b/core/profiles/minimal/config/install/block.block.stark_admin.yml
   @@ -1,21 +1,25 @@
   +provider: null
   

   Don't profiles count as modules? All of these say null.

+++ b/core/modules/system/src/Tests/Installer/MinimalInstallerTest.php
@@ -0,0 +1,64 @@
+    // \Drupal\filter\Entity\FilterFormat::toArray() drops the roles of filter
+    // formats.
+//    $skipped_config['filter.format.basic_html'][] = 'roles:';
+//    $skipped_config['filter.format.basic_html'][] = ' - authenticated';
+//    $skipped_config['filter.format.full_html'][] = 'roles:';
+//    $skipped_config['filter.format.full_html'][] = ' - administrator';
+//    $skipped_config['filter.format.restricted_html'][] = 'roles:';
+//    $skipped_config['filter.format.restricted_html'][] = ' - anonymous';

?

re #28.2 ConfigEntityBase::toArray() removes empty third party settings. I've debated creating an issue to do the same for dependencies many time.

Ah good to know. Thanks alex.

Don't profiles count as modules? All of these say null.

No idea why this happens, but well, its what is there. I guess what we want to do is to actually remove 'provider' from Block entity as its not used at all?

For now this just addresses the feedback + redness of the patch

Expanding the issue summary to explain the steps we need to take care off, together with some ideas how to improve the reviewability of this patch and what to do as well.
Have fun!

• Fixed the test failures
• Also exported the optional config.

No testbot you are drunken!

working on fails

@dawehner pointed out there are no failures, pifr bot is playing up

"database or disk is full"

Well, let's ignore the old testbot then.

<strike> -> <s> in the issue summary's remaining tasks, to make it actually clear what has already been done without reading HTML :P

d.o does not allow <s>, but does allow <del>. Oh, d.o…

I reviewed the entire patch, including looking for things mentioned in this part of the remaining tasks:

Currently the patch contains mainly out of two parts: a) reordering of existing config b) adding defaults values back. In order to make it easier to review the first patch we could do the following: Write a script which looks at all the config files in HEAD and sorts it strictly alphabetically. We then could do the same with the files after this patch.
When we then compare these two sets of files we have excluded the orderings and the only remaining bits are additions/removals.

Though I would like official confirmation that we do special case langcode, status, dependencies, id and label. Those five are always listed first, and always in that order.

1. +++ b/core/modules/aggregator/config/install/core.entity_view_display.aggregator_feed.aggregator_feed.summary.yml
   @@ -1,22 +1,22 @@
   +status: true
   ...
   -status: true
   ...
   -status: true
   

   Nice, from multiple to one :)

2. +++ b/core/modules/aggregator/config/optional/views.view.aggregator_rss_feed.yml
   @@ -129,6 +129,14 @@ display:
   +    cache_metadata:
   +      contexts:
   +        - 'languages:language_content'
   +        - 'languages:language_interface'
   +        - url.query_args
   +        - user.permissions
   +      cacheable: false
   

   /me points at #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface with a sad expression, which was RTBC >2.5 months ago, and has since been blocked on upgrade path criticals that it kept uncovering.

   We'll have to re-export all Views config such as this one after that lands.

3. +++ b/core/modules/forum/config/install/field.field.taxonomy_term.forums.forum_container.yml
   @@ -17,7 +17,6 @@ default_value:
   -  on_label: Yes
   -  off_label: No
   ...
   +  on_label: 'Yes'
   +  off_label: 'No'
   

   Hrm, strange… elsewhere, we don't see single quotes around strings IF those strings don't contain spaces.

   That seems inconsistent?

4. +++ b/core/modules/forum/config/install/field.storage.node.taxonomy_forums.yml
   @@ -10,7 +10,7 @@ entity_type: node
   -module: taxonomy
   +module: entity_reference
   

   Nice catch!

   Though I wonder if this shouldn't be forum module that owns this?

5. +++ b/core/modules/language/config/install/language.entity.zxx.yml
   @@ -1,7 +1,8 @@
   +direction: ltr
   +weight: 3
    locked: true
   

   This is not alphabetically sorted.

6. +++ b/core/modules/node/config/install/core.entity_view_mode.node.teaser.yml
   @@ -1,8 +1,9 @@
   +targetEntityType: node
   +cache: true
   

   Not sorted alphabetically.

7. +++ b/core/modules/node/config/install/system.action.node_delete_action.yml
   @@ -1,9 +1,10 @@
   +type: node
   +plugin: node_delete_action
   +configuration: {  }
   

   Not sorted alphabetically.

8. +++ b/core/modules/node/config/optional/views.view.archive.yml
   @@ -205,3 +223,12 @@ display:
   +        - 'languages:language_interface'
   +        - url
   

   So it also seems we add quotes if there is a colon in the string. I wonder why?

   Does YAML assign special meaning to string values with colons? Could it for example be something namespaced?

   Just asking, because it'd be more legible if this were not quoted. Or otherwise, if it all were consistently quoted.

9. +++ b/core/modules/system/config/install/core.date_format.html_year.yml
   @@ -1,6 +1,7 @@
   +pattern: 'Y'
   

   Why the quotes?

10. +++ b/core/modules/system/src/Tests/Installer/MinimalInstallerTest.php
    @@ -0,0 +1,55 @@
    +class MinimalInstallerTest extends InstallerTestBase {
    ...
    +  public function testMinimalConfig() {
    

    I wonder if we don't just want to make a new base class that extends InstallerTestBase, which can then be used to test every profile without repeating this.

11. +++ b/core/modules/system/src/Tests/Installer/MinimalInstallerTest.php
    @@ -0,0 +1,55 @@
    +    $profile = 'minimal';
    

    s/$profile/$this->profile/

12. +++ b/core/modules/system/src/Tests/Installer/MinimalInstallerTest.php
    @@ -0,0 +1,55 @@
    +  }
    +}
    

    Nit: missing newline in between.

13. +++ b/core/modules/system/src/Tests/Installer/StandardInstallerTest.php
    @@ -42,5 +48,42 @@ protected function setUpSite() {
    +    $profile = 'standard';
    

    s/$profile/$this->profile/

14. +++ b/core/modules/system/src/Tests/Installer/StandardInstallerTest.php
    @@ -42,5 +48,42 @@ protected function setUpSite() {
    +    // \Drupal\simpletest\WebTestBase::installParameters() uses
    +    // simpletest@example.com as mail address.
    +    $skipped_config['contact.form.feedback'][] = ' - simpletest@example.com';
    

    This one I understand.

15. +++ b/core/modules/system/src/Tests/Installer/StandardInstallerTest.php
    @@ -42,5 +48,42 @@ protected function setUpSite() {
    +    // \Drupal\filter\Entity\FilterFormat::toArray() drops the roles of filter
    +    // formats.
    +    $skipped_config['filter.format.basic_html'][] = 'roles:';
    +    $skipped_config['filter.format.basic_html'][] = ' - authenticated';
    +    $skipped_config['filter.format.full_html'][] = 'roles:';
    +    $skipped_config['filter.format.full_html'][] = ' - administrator';
    +    $skipped_config['filter.format.restricted_html'][] = 'roles:';
    +    $skipped_config['filter.format.restricted_html'][] = ' - anonymous';
    

    This one I don't understand.

    Oh, it's because of

        // The 'roles' property is only used during install and should never
        // actually be saved.
        unset($properties['roles']);
    

    in \Drupal\filter\Entity\FilterFormat::toArray(). Which is … bizarre, but definitely out of scope of this issue to fix.

16. +++ b/core/profiles/minimal/config/install/block.block.stark_admin.yml
    @@ -1,21 +1,25 @@
     region: sidebar_first
    +weight: 1
    +provider: null
     plugin: 'system_menu_block:admin'
     settings:
    

    Not sorted alphabetically.

17. +++ b/core/profiles/minimal/config/install/block.block.stark_local_actions.yml
    @@ -1,15 +1,19 @@
    +provider: null
     plugin: local_actions_block
     settings:
       id: local_actions_block
    -  label: Primary admin actions
    +  label: 'Primary admin actions'
    +  provider: core
    

    Huh, a provider not at the top level but in settings?

    Apparently this is a thing, see block_settings in core.data_types.schema.yml. So even if this is wrong, fixing it is out of scope here.

18. +++ b/core/profiles/standard/config/install/core.entity_form_display.comment.comment.default.yml
    @@ -27,5 +20,11 @@ content:
    +    third_party_settings: {  }
     hidden: {  }
    -third_party_settings: {  }
    

    It's fascinating that top-level third_party_settings are omitted by default, but nested ones aren't.

19. +++ b/core/profiles/standard/config/install/core.entity_view_display.node.article.teaser.yml
    @@ -43,8 +42,6 @@ content:
     hidden:
    -  langcode: true
    
    +++ b/core/profiles/standard/config/install/core.entity_view_display.node.page.default.yml
    @@ -20,6 +20,4 @@ content:
    -hidden:
    -  langcode: true
    ...
    +hidden: {  }
    
    +++ b/core/profiles/standard/config/install/core.entity_view_display.node.page.teaser.yml
    @@ -22,6 +22,4 @@ content:
    -hidden:
    -  langcode: true
    ...
    +hidden: {  }
    

    Why are these changes correct? Is there something else that is causing langcode to be hidden? Or is it hidden by default?

20. +++ b/core/profiles/standard/config/install/editor.editor.full_html.yml
    @@ -12,7 +19,7 @@ settings:
    -            - -
    +            - '-'
    

    Hah! That makes sense :)

21. +++ b/core/profiles/standard/config/install/field.field.node.article.field_tags.yml
    @@ -1,26 +1,27 @@
    -  handler: default
    +  handler: 'default:taxonomy_term'
    

    This is another interesting change. We should double-check that this is correct.

    Also, again the strange quoting thing (quotes when colon is present).

22. +++ b/core/tests/Drupal/KernelTests/AssertConfigTrait.php
    @@ -0,0 +1,86 @@
    +   * @param \Drupal\Component\Diff\Diff $result
    +   * @param $config_name
    +   * @param array $skipped_config
    

    Incomplete docs.

23. +++ b/core/tests/Drupal/KernelTests/AssertConfigTrait.php
    @@ -0,0 +1,86 @@
    +          // Its not part of the skipped config, so we can directly throw the
    

    s/Its/It's/
    or
    s/Its/It is/

24. +++ b/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php
    @@ -0,0 +1,112 @@
    +class DefaultConfigTest extends KernelTestBase {
    ...
    +   * @dataProvider providerTestModuleConfig
    

    This first confused me, but then I realized this is a KernelTestBaseNG test. Nice! :)

25. +++ b/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php
    @@ -0,0 +1,112 @@
    +    // The following config entries are changed on module install, so compare
    +    // it doesn't make sense.
    

    s/compare it/comparing them/

We'll have to re-export all Views config such as this one after that lands.

Yeah, that is life.

Hrm, strange… elsewhere, we don't see single quotes around strings IF those strings don't contain spaces.

Well, this happened automatically. Not sure entirely why to be hoenst though.

+++ b/core/modules/forum/config/install/field.storage.node.taxonomy_forums.yml
@@ -10,7 +10,7 @@ entity_type: node
-module: taxonomy
+module: entity_reference

Nice catch!

Though I wonder if this shouldn't be forum module that owns this?

Well, this all happened automatically, so yeah this is the right thing do, as it seems. the module is probably the module of the field type.

This is not alphabetically sorted.

But this is not the point. Its exported how the config system saves stuff (see '* config_export = {' for example). Just have a look at

+langcode: en
+status: true
+dependencies:

... its not sorted in any western alphabet I am aware of.

So it also seems we add quotes if there is a colon in the string. I wonder why?

Does YAML assign special meaning to string values with colons? Could it for example be something namespaced?

Just asking, because it'd be more legible if this were not quoted. Or otherwise, if it all were consistently quoted.

Seems so, at least this is how your YML parser/writer works.

I wonder if we don't just want to make a new base class that extends InstallerTestBase, which can then be used to test every profile without repeating this.

Yeah, why not, but I guess we would not save much.

Which is … bizarre, but definitely out of scope of this issue to fix.

Indeed. You know, its documented right above there :)

Why are these changes correct? Is there something else that is causing langcode to be hidden? Or is it hidden by default?

If you save such a thing without language module enable, it is not there. Well we can readd them, but I think its not needed.

This is another interesting change. We should double-check that this is correct.

Well, this also happened automatically and IMHO makes sense.

This first confused me, but then I realized this is a KernelTestBaseNG test. Nice! :)

It is not that bad, right?

Why are these changes correct? Is there something else that is causing langcode to be hidden? Or is it hidden by default?

Note: Without those removals, they are removed on install.

Addressed the feedback from wim.

Nice work.

+++ b/core/profiles/standard/config/install/filter.format.full_html.yml
@@ -1,6 +1,10 @@
 roles:
   - administrator

Should filter.format.full_html have a config dependency on user.role.administrator? Ditto elsewhere when anonymous and authenticated roles (provided by user module) are listed.

Good observation! Opened an issue for that: #2569741: [PP-1] FilterFormat should add the roles as config dependencies

Merged this with #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available which has tests that ensure our YAML is valid for both symfony/yaml and the PECL YAML extension.

Passed all tests - which means we're not introducing any regressions here on that front.

@larowlan
--group Serialization?

Filed #2571187: EntityReferenceItem should add the target_bundles as config dependencies for what looked like another missing config dependency.

Going to review.

Hrm, strange… elsewhere, we don't see single quotes around strings IF those strings don't contain spaces.

Well, this happened automatically. Not sure entirely why to be hoenst though.

So it also seems we add quotes if there is a colon in the string. I wonder why?

Does YAML assign special meaning to string values with colons? Could it for example be something namespaced?

Just asking, because it'd be more legible if this were not quoted. Or otherwise, if it all were consistently quoted.

Seems so, at least this is how your YML parser/writer works.

These are indeed quoted by the Yaml dumper. Symfony\Component\Yaml\Dumper::dump() is the entry point, but the most relevant parts are:

• \Symfony\Component\Yaml\Inline::dump() does some basic type checks for booleans, numeric values etc.
• \Symfony\Component\Yaml\Escaper::requiresDoubleQuoting() checks when double quotes are needed. This seems to be for very specific ASCII characters, the list contains things like [\\x00-\\x1f] etc.
• \Symfony\Component\Yaml\Escaper::requiresSingleQuoting() is the one that is responsible for deciding when single quotes are used. Indeed if whitespace, a colon, a "Y" or "N" or a bunch of other symbols is present in the string then it will be quoted. Here's the full code for reference:
  

      public static function requiresSingleQuoting($value)
      {
          // Determines if a PHP value is entirely composed of a value that would
          // require single quoting in YAML.
          if (in_array(strtolower($value), array('null', '~', 'true', 'false', 'y', 'n', 'yes', 'no', 'on', 'off'))) {
              return true;
          }
  
          // Determines if the PHP value contains any single characters that would
          // cause it to require single quoting in YAML.
          return preg_match('/[ \s \' " \: \{ \} \[ \] , & \* \# \?] | \A[ \- ? | < > = ! % @ ` ]/x', $value);
      }
  

#72 thanks for clarifying — all quite strange though! I guess it's part of the YML spec?

I was going through all exported configuration but was just hitting the same things that @Wim Leers remarked earlier. It is very tiring, big respect for Wim to power through the whole thing!

I thought of creating the script to order all config alphabetically but discussed this in IRC and Wim didn't think it necessary any more since he reviewed all of it anyway.

I'll skip the rest of the exported config and go through the test coverage and the latest fixes.

1. +++ b/core/modules/system/src/Tests/Installer/ConfigAfterInstallerTestBase.php
   @@ -0,0 +1,49 @@
   +      try {
   +        $this->assertConfigDiff($result, $config_name, $skipped_config);
   +      }
   +      catch (\Exception $e) {
   +        $this->fail($e->getMessage());
   +      }
   

   Is this a common pattern, to throw an exception in the assert in the trait, then catching the exception and converting it in a fail?

   Wouldn't it be easier just to call $this->fail()? Maybe this is done so to make it compatible with all possible test systems?

2. +++ b/core/tests/Drupal/KernelTests/AssertConfigTrait.php
   @@ -0,0 +1,92 @@
   +   *   An array of skipped config, keyed by string. If the value is TRUE, the
   +   *   entire file will be ignored, otherwise its an array of strings which are
   +   *   ignored.
   

   Typo: "its an array" -> "it's an array".

Is this a common pattern, to throw an exception in the assert in the trait, then catching the exception and converting it in a fail?

Wouldn't it be easier just to call $this->fail()? Maybe this is done so to make it compatible with all possible test systems?

Well, the thing is, for kernel/unit tests we want to throw an exception. Stop as early as possible and be done.
That try{} / catch {} was my way to then cause a test failure in a simpletest.

There we go.

I've been looking into the DefaultConfigTest failure. It seems like there is nothing inherently wrong with the test, it is simply timing out.

Trying to reset the timeout at the start of every test. Discussed it with @dawehner, he thinks it might be the test runner timing out, not the tests itself.

I'm curious whether this is the thing what we should fix.

Sadly @pfrenssen failed to merge in the interdiff of #79

Sorry I indeed forgot to merge in the interdiff, good that you saw it!

Had a quick look at this and don't see any obvious problems - in talking to @dawehner and @pfrenssen they seemed optimistic about this being close to RTBC. I think this is close as well, considering the patch is green and @Wim Leers has done an extensive review of the actual config already.

+++ b/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php
@@ -0,0 +1,123 @@
+    // @todo Extra hack to avoid test fails, remove this once
+    //   https://www.drupal.org/node/2553661 is fixed.
+    FileCacheFactory::setPrefix(Settings::getApcuPrefix('file_cache', $this->root));

This should not be needed anymore

Good catch @stefan.r

Rebased

#2458763: Remove the ability to configure a block's cache max-age caused this failure.

Have reviewed this a few times as it's been going along, and don't have any comments.

I think it's important to get this in, now that it is we need to try to keep the exported config up-to-date when other patches land if possible.

Committed/pushed to 8.0.x, thanks!

Yeah we now have tests so it will be ensured at least.

Yeah we now have tests so it will be ensured at least.

Exactly! This is now one of my favorite tests for that very reason :)

+++ b/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php
@@ -0,0 +1,119 @@
+    // @todo Figure out why simpletest.settings is not installed.

Can we have a follow up for this?