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.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

kalpaitch created an issue. See original summary.

jmmarquez’s picture

Assigned: Unassigned » jmmarquez

I am working on it!!

jmmarquez’s picture

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

I'm still reviewing it

kalpaitch’s picture

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

kalpaitch’s picture

FileSize
1.9 KB

Oops, both patches together then...

jmmarquez’s picture

Ok, so it should be fixed now?

kalpaitch’s picture

Status: Needs work » Needs review

Supposedly :)

jmmarquez’s picture

jmmarquez’s picture

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

Issue tags: +Novice

bump

borisson_’s picture

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

Status: Needs work » Needs review

Hi borisson,

I think code is correct.

jeetendrakumar’s picture

Status: Needs review » Reviewed & tested by the community

code is working fine for me. Good Job !

borisson_’s picture

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

xjm’s picture

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

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.