Problem/Motivation

Whilst discussing #2316909: Revisit all built-in test/default views configuration in core with @catch, @effulgentsia, @webchick and @xjm since it was tagged with "revisit before release candidate", we decided that we need to ensure that the configuration shipped with core in yml files matches the configuration in the active store after it is installed. This is important since incorrect default configuration can cause unexpected bugs and confusion for developers.

Proposed resolution

Revisit all configuration and make sure auto generated with latest config/plugin/schema changes.

This was done before whilst creating many of the configuration schema. For example, #2316909: Revisit all built-in test/default views configuration in core and the beginnings of a script to do this is available at https://gist.github.com/alexpott/f0650fbb7954df8497e4

Remaining tasks

  1. Discuss what we should do about config provided by test modules
  2. Re-export the files in the installation profile
  3. Adapt the config_refresh module to also export optional default config, see #2568459: Configurations in config/optional is not getting updated
  4. Use the former to also export all optional config in core/modules core/profiles/(standard|minimal)
  5. 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.
  6. Review

User interface changes

None

API changes

None

Data model changes

Should be none

Files: 
CommentFileSizeAuthor
#92 interdiff.txt13.89 KBdawehner
#92 2567183-92.patch143.31 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,864 pass(es). View
#90 2567183-90.patch144.13 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,266 pass(es), 64 fail(s), and 22 exception(s). View
#88 interdiff.txt0 bytesdawehner
#88 2567183-88.patch144.33 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2567183-88.patch. Unable to apply patch. See the log in the details link for more information. View
#84 interdiff.txt2.76 KBdawehner
#84 2567183-82.patch144.54 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,732 pass(es). View
#81 interdiff.txt3.51 KBpfrenssen
#81 2567183-81.patch143.06 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,668 pass(es), 1 fail(s), and 0 exception(s). View
#65 interdiff.txt7.27 KBdawehner
#65 2567183-65.patch142.71 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,629 pass(es), 1 fail(s), and 0 exception(s). View
#49 interdiff.txt23.87 KBdawehner
#49 2567183-49.patch142.34 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,275 pass(es). View
#45 interdiff.txt2.77 KBdawehner
#45 2568387-45.patch119.41 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,079 pass(es), 1 fail(s), and 0 exception(s). View
#39 interdiff.txt10.24 KBdawehner
#39 2567183-38.patch120.38 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,995 pass(es), 3 fail(s), and 0 exception(s). View
#33 interdiff.txt9.75 KBdawehner
#33 2567183-31.patch112.36 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,945 pass(es), 2 fail(s), and 0 exception(s). View
#30 interdiff.txt3.1 KBdawehner
#30 2567183-30.patch111.65 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,909 pass(es), 2 fail(s), and 0 exception(s). View
#29 interdiff.txt60.75 KBdawehner
#29 2567183-29.patch113.65 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,903 pass(es), 2 fail(s), and 0 exception(s). View
#25 interdiff.txt7.52 KBdawehner
#25 2567183-25.patch58.49 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,713 pass(es). View
#23 interdiff.txt6.06 KBdawehner
#23 2567183-23.patch58.9 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,650 pass(es). View
#19 2567183-19.patch58.4 KBhussainweb
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,574 pass(es), 1 fail(s), and 0 exception(s). View
#19 interdiff-15-19.txt1023 byteshussainweb
#15 interdiff.txt13.42 KBdawehner
#15 2567183-12.patch58.59 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,576 pass(es), 6 fail(s), and 0 exception(s). View
#11 2567183-11.patch52.39 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,577 pass(es), 5 fail(s), and 0 exception(s). View

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Issue summary: View changes
iantresman’s picture

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

alexpott’s picture

@iantresman nope. This is about ensuring that things like core/modules/node/config/install/node.settings.yml and core/modules/node/config/install/node.settings.yml are not unexpectedly changed during installation.

catch’s picture

Title: Revisit all built-in configuration in core » Re-export all built-in configuration in core

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

dawehner’s picture

Component: views.module » configuration system

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

jibran’s picture

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

vijaycs85’s picture

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 :(

webchick’s picture

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

dawehner’s picture

I'll give it a try for a while.

dawehner’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
52.39 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,577 pass(es), 5 fail(s), and 0 exception(s). View

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:

Status: Needs review » Needs work

The last submitted patch, 11: 2567183-11.patch, failed testing.

aspilicious’s picture

Should config in the installation folder contain a uuid?

The last submitted patch, 11: 2567183-11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
58.59 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,576 pass(es), 6 fail(s), and 0 exception(s). View
13.42 KB

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

Status: Needs review » Needs work

The last submitted patch, 15: 2567183-12.patch, failed testing.

The last submitted patch, 15: 2567183-12.patch, failed testing.

vijaycs85’s picture

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

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1023 bytes
58.4 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,574 pass(es), 1 fail(s), and 0 exception(s). View

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.

hussainweb’s picture

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

Status: Needs review » Needs work

The last submitted patch, 19: 2567183-19.patch, failed testing.

The last submitted patch, 19: 2567183-19.patch, failed testing.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
58.9 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,650 pass(es). View
6.06 KB

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.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/filter/config/install/filter.format.plain_text.yml
@@ -1,23 +1,16 @@
-# Every site requires at least one text format as fallback format that
-# - is accessible to all users.
-# - is secure, using very basic formatting only.
-# - may be modified by installation profiles to have other properties.

We need to preserve comments.

dawehner’s picture

Status: Needs work » Needs review
FileSize
58.49 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,713 pass(es). View
7.52 KB

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

dawehner’s picture

dawehner’s picture

Issue summary: View changes
Status: Needs review » Needs work

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

Wim Leers’s picture

  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?

dawehner’s picture

Status: Needs work » Needs review
FileSize
113.65 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,903 pass(es), 2 fail(s), and 0 exception(s). View
60.75 KB

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.

dawehner’s picture

FileSize
111.65 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,909 pass(es), 2 fail(s), and 0 exception(s). View
3.1 KB

Just

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

for now.

The last submitted patch, 29: 2567183-29.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 30: 2567183-30.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
112.36 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,945 pass(es), 2 fail(s), and 0 exception(s). View
9.75 KB

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

dawehner’s picture

Issue summary: View changes
Issue tags: -Needs tests

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

Status: Needs review » Needs work

The last submitted patch, 33: 2567183-31.patch, failed testing.

The last submitted patch, 29: 2567183-29.patch, failed testing.

The last submitted patch, 30: 2567183-30.patch, failed testing.

The last submitted patch, 33: 2567183-31.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
120.38 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,995 pass(es), 3 fail(s), and 0 exception(s). View
10.24 KB

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

tim.plunkett’s picture

  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.

Wim Leers’s picture

+++ 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';

?

Status: Needs review » Needs work

The last submitted patch, 39: 2567183-38.patch, failed testing.

The last submitted patch, 39: 2567183-38.patch, failed testing.

alexpott’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
119.41 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,079 pass(es), 1 fail(s), and 0 exception(s). View
2.77 KB

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

Status: Needs review » Needs work

The last submitted patch, 45: 2568387-45.patch, failed testing.

dawehner’s picture

Issue summary: View changes

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!

The last submitted patch, 45: 2568387-45.patch, failed testing.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
142.34 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,275 pass(es). View
23.87 KB
  • Fixed the test failures
  • Also exported the optional config.

Status: Needs review » Needs work

The last submitted patch, 49: 2567183-49.patch, failed testing.

dawehner queued 49: 2567183-49.patch for re-testing.

The last submitted patch, 49: 2567183-49.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

No testbot you are drunken!

dawehner queued 49: 2567183-49.patch for re-testing.

The last submitted patch, 49: 2567183-49.patch, failed testing.

larowlan’s picture

Assigned: Unassigned » larowlan

working on fails

larowlan’s picture

Assigned: larowlan » Unassigned

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

larowlan’s picture

"database or disk is full"

dawehner’s picture

Status: Needs work » Needs review

Well, let's ignore the old testbot then.

Wim Leers’s picture

Issue summary: View changes

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

Wim Leers’s picture

Issue summary: View changes

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

dawehner queued 49: 2567183-49.patch for re-testing.

Wim Leers’s picture

Status: Needs review » Needs work

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/

dawehner’s picture

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?

dawehner’s picture

Status: Needs work » Needs review
FileSize
142.71 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,629 pass(es), 1 fail(s), and 0 exception(s). View
7.27 KB

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.

nedjo’s picture

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.

dawehner’s picture

Good observation! Opened an issue for that: #2569741: FilterFormat should add the roles as config dependencies

larowlan’s picture

Issue summary: View changes
FileSize
97.07 KB

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.

dawehner’s picture

@larowlan
--group Serialization?

nedjo’s picture

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

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

Going to review.

pfrenssen’s picture

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);
        }
    
Wim Leers’s picture

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

pfrenssen’s picture

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.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
  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".

dawehner’s picture

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.

pfrenssen queued 65: 2567183-65.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 65: 2567183-65.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
143.71 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,694 pass(es), 15 fail(s), and 3 exception(s). View
1 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, 79: 2567183-79.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
143.06 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,668 pass(es), 1 fail(s), and 0 exception(s). View
3.51 KB

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.

The last submitted patch, 79: 2567183-79.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 81: 2567183-81.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
144.54 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,732 pass(es). View
2.76 KB

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

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

The last submitted patch, 81: 2567183-81.patch, failed testing.

pfrenssen’s picture

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

stefan.r’s picture

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

dawehner’s picture

FileSize
144.33 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2567183-88.patch. Unable to apply patch. See the log in the details link for more information. View
0 bytes

Good catch @stefan.r

Status: Needs review » Needs work

The last submitted patch, 88: 2567183-88.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
144.13 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,266 pass(es), 64 fail(s), and 22 exception(s). View

Rebased

Status: Needs review » Needs work

The last submitted patch, 90: 2567183-90.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
143.31 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,864 pass(es). View
13.89 KB

The last submitted patch, 88: 2567183-88.patch, failed testing.

The last submitted patch, 90: 2567183-90.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • catch committed c1aa46f on 8.0.x
    Issue #2567183 by dawehner, pfrenssen, hussainweb: Re-export all built-...
dawehner’s picture

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

Wim Leers’s picture

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 :)

jibran’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.