The error thrown when the profile for an already installed site is changed in config is misleading: 'from %new_profile to %profile'. Took me a longer to figure what was going on because of this.

CommentFileSizeAuthor
#7 config_import_change-2862207-7.patch1.9 KBkalpaitch
#6 config_import_change-2862207-6.patch984 byteskalpaitch
#3 Config_import_change_profile_message-2862207-03.patch959 bytesjmmarquez
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kalpaitch created an issue. See original summary.

jmmarquez’s picture

jmmarquez CreditAttribution: jmmarquez commented
Assigned: Unassigned » jmmarquez

I am working on it!!

jmmarquez’s picture

jmmarquez CreditAttribution: jmmarquez commented
Status: Active » Needs review
FileSize
Config_import_change_profile_message-2862207-03.patch959 bytes

Uploading patch.

Status: Needs review » Needs work

The last submitted patch, 3: Config_import_change_profile_message-2862207-03.patch, failed testing.

jmmarquez’s picture

jmmarquez CreditAttribution: jmmarquez commented

I'm still reviewing it

kalpaitch’s picture

kalpaitch CreditAttribution: kalpaitch as a volunteer and commented
FileSize
config_import_change-2862207-6.patch984 bytes

Good good, just needs the same amendment to the tests.

kalpaitch’s picture

kalpaitch CreditAttribution: kalpaitch as a volunteer and commented
FileSize
config_import_change-2862207-7.patch1.9 KB

Oops, both patches together then...

jmmarquez’s picture

jmmarquez CreditAttribution: jmmarquez commented

Ok, so it should be fixed now?

kalpaitch’s picture

kalpaitch CreditAttribution: kalpaitch as a volunteer and commented
Status: Needs work » Needs review

Supposedly :)

jmmarquez’s picture

jmmarquez CreditAttribution: jmmarquez at La Drupalera by Emergya for La Drupalera by Emergya commented
jmmarquez’s picture

jmmarquez CreditAttribution: jmmarquez at La Drupalera by Emergya for La Drupalera by Emergya commented
Assigned: jmmarquez » Unassigned

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kalpaitch’s picture

kalpaitch CreditAttribution: kalpaitch as a volunteer and commented
Issue tags: +Novice

bump

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
@@ -136,7 +136,7 @@ protected function validateModules(ConfigImporter $config_importer) {
+      $config_importer->logError($this->t('Cannot change the install profile from %profile to %new_profile once Drupal is installed.', ['%profile' => $install_profile, '%new_profile' => $core_extension['profile']]));

Instead of switching the %profile and %new_profile around, we should switch the variable around in the arguments intead.

jeetendrakumar’s picture

jeetendrakumar CreditAttribution: jeetendrakumar as a volunteer and at HyTechPro.com commented
Status: Needs work » Needs review

Hi borisson,

I think code is correct.

jeetendrakumar’s picture

jeetendrakumar CreditAttribution: jeetendrakumar as a volunteer and at HyTechPro.com commented
Status: Needs review » Reviewed & tested by the community

code is working fine for me. Good Job !

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented

Oh wow, I completely misread the interdiff, yeah this does look correct. Thanks for pointing that out @jeetendrakumar!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented 
+++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php
@@ -692,7 +692,7 @@ public function testInstallProfileMisMatch() {
       // does not use an install profile. This situation should be impossible
       // to get in but site's can removed the install profile setting from
       // settings.php so the test is valid.
-      $this->assertEqual(['Cannot change the install profile from <em class="placeholder">this_will_not_work</em> to <em class="placeholder"></em> once Drupal is installed.'], $error_log);
+      $this->assertEqual(['Cannot change the install profile from <em class="placeholder"></em> to <em class="placeholder">this_will_not_work</em> once Drupal is installed.'], $error_log);

Based on the comment, it looks like the message should also be different if the install profile name is not set. Otherwise the message is still confusing.

That is not in scope for this bugfix, though, so I filed #2929505: Fix config importer error message when a profile name is empty.

Saving issue credit.

  • xjm committed e4c874e on 8.5.x 
    Issue #2862207 by kalpaitch, jmmarquez, jeetendrakumar: Config import...

  • xjm committed 1195316 on 8.4.x 
    Issue #2862207 by kalpaitch, jmmarquez, jeetendrakumar: Config import...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +String change in 8.4.4

Normally, string changes aren't allowed changes in patch releases. However, in this situation, the string is purely wrong, so it is better to require re-translation than to keep the old string. So, tagging as a string change in the next patch release.

I think this is also a normal issue (not minor) because it is a functional bug and does affect debugging for config imports (as described in the summary).

Committed and pushed to 8.5.x, and backported to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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