Problem/Motivation

This is the Umami version of #2990776: Remove config-editing parts from standard_install() in favor of exported configuration.

demo_umami_install() manually updates various bits of configuration instead of shipping with proper configuration exports in config/install. This promotes a bad practice for other installation profile authors.

Proposed resolution

Remove all configuration updates in favor of exported configuration files.

CommentFileSizeAuthor
#14 interdiff.txt1.26 KBlongwave
#14 2998483-14.patch6.24 KBlongwave
#11 interdiff.txt1.75 KBlongwave
#11 2998483-11.patch5.38 KBlongwave
#9 umami_demo-d8-remove_config_editing-2.patch5.36 KBiuana
#4 umami_demo-d8-remove_config_editing.patch5.54 KBiuana
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

iuana’s picture

iuana CreditAttribution: iuana at Softescu commented
Assigned: Unassigned » iuana
chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Issue tags: +Novice, +DrupalEurope
iuana’s picture

iuana CreditAttribution: iuana at Softescu commented
Assigned: iuana » Unassigned
Status: Active » Needs review
FileSize
umami_demo-d8-remove_config_editing.patch5.54 KB

Hi, here are my changes regarding removing config editing from hook_install().

Status: Needs review » Needs work

The last submitted patch, 4: umami_demo-d8-remove_config_editing.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Status: Needs work » Needs review

I'm not sure if that's why the tests are failing but we shouldn't have the UUIDs at the top of the new config files.

We should also remove

use Drupal\user\RoleInterface;

from profiles/demo_umami/demo_umami.install, as we are no longer using it, as suggested by the codesniffer_fixes.patch file in #5

iuana’s picture

iuana CreditAttribution: iuana at Softescu commented
Assigned: Unassigned » iuana
iuana’s picture

iuana CreditAttribution: iuana at Softescu commented
iuana’s picture

iuana CreditAttribution: iuana at Softescu commented
Assigned: iuana » Unassigned
FileSize
umami_demo-d8-remove_config_editing-2.patch5.36 KB

Indeed, thanks

Status: Needs review » Needs work

The last submitted patch, 9: umami_demo-d8-remove_config_editing-2.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave as a volunteer commented
Status: Needs work » Needs review
FileSize
2998483-11.patch5.38 KB
interdiff.txt1.75 KB

A few more config entries were needed to get the tests to pass: the site name and email and some extra permissions, and the default_config_hash isn't required,

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Status: Needs review » Reviewed & tested by the community

Have reviewed and confirmed all the removed code from demo_umami_install() is reflected in configuration.
Have checked that configuration against exports from a site without the patch and confirmed it does match.
Have run SimplyTest.me with this patch and visually inspected all the configurations moved from code to config do match.

Therefore marking RTBC.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Reviewed & tested by the community » Needs work

Looked through the patch and compared it to the one committed over in #2990776: Remove config-editing parts from standard_install() in favor of exported configuration. Really great work, it looks almost perfect! Thanks everyone for working on this. However, I found one thing, which still needs work before this can committed:

+++ b/core/profiles/demo_umami/config/install/system.site.yml
@@ -0,0 +1,11 @@
+name: Drupal
+mail: admin@example.com

First of all there should be a uuid key as the first entry with an empty string as a value.

The name and mail entries should be set to empty strings, as well. We should not harcode the site name and e-mail address for everyone.

The tests were failing because SimpleTest overrides these values when installing Drupal. So we need to add something like the following to the third parameter of ::assertConfigDiff() in DemoUmamiProfileTest::assertDefaultConfig():

'system.site' => [
  'name: Drupal',
  'mail: simpletest@example.com',
  'uuid: ' . $this->config('system.site')->get('uuid'),
],

That way the test should pass again.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave as a volunteer commented
Status: Needs work » Needs review
FileSize
2998483-14.patch6.24 KB
interdiff.txt1.26 KB

Thanks for the review. I fixed everything mentioned in #13 but could not get assertDefaultConfig() to pass with the exact values (not sure why), however it allows substrings so I just listed the keys and this seems to work.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Perfect, that's event better! Thanks @longwave.

Thinking about this, I think the reason is that assertDefaultConfig() ignores the respective parts from the default config, not the active config - while the respective test for Standard profile does it the other way around.

In any case, let's do this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed e4f5c03c92 to 8.7.x and 23ae4779af to 8.6.x. Thanks!

diff --git a/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php b/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php
index 99d9512557..e27c6a19a3 100644
--- a/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php
+++ b/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php
@@ -79,7 +79,7 @@ protected function assertDefaultConfig(StorageInterface $default_config_storage,
           'filter.format.restricted_html' => ['roles:', '  - anonymous'],
           // The system.site config is overwritten during tests by
           // FunctionalTestSetupTrait::installParameters().
-          'system.site' => ['uuid:', 'name:', 'mail:']
+          'system.site' => ['uuid:', 'name:', 'mail:'],
         ]);
       }
       else {

Fix array coding standards on commit.

  • alexpott committed e4f5c03 on 8.7.x 
    Issue #2998483 by longwave, iuana, tstoeckler, Eli-T: Remove config-...

  • alexpott committed 23ae477 on 8.6.x 
    Issue #2998483 by longwave, iuana, tstoeckler, Eli-T: Remove config-...

Status: Fixed » Closed (fixed)

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