Discovered in #2466197: Staging directory should not have to be writeable.

Problem/Motivation

\Drupal\Core\Config\FileStorageFactory::getSync() just throws a \Exception when the config directory type does not exist - that's way too generic.

Proposed resolution

Create a more specific exception.

Remaining tasks

  • Create new exception type
  • Review and commit

User interface changes

None

API changes

A new exception - but any code catching the existing exception will work because the new exception will extend from \Exception

CommentFileSizeAuthor
#28 config-2696103-28.patch2.54 KBDerekCresswell
#24 config-2696103-24.patch1.58 KBDerekCresswell
#22 2696103-22.patch1.69 KBashok.gharpankar
#11 2696103-11.patch2.39 KBchOP
#4 2696103-4.patch1.38 KBGabr1el
#4 interdiff.txt1.05 KBGabr1el
#2 2696103-2.patch1.08 KBGabr1el
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

Gabr1el’s picture

Gabr1el CreditAttribution: Gabr1el for ASSIST Software SRL commented
Status: Active » Needs review
FileSize
2696103-2.patch1.08 KB

Added a custom exception class and used it in config_get_config_directory().

Status: Needs review » Needs work

The last submitted patch, 2: 2696103-2.patch, failed testing.

Gabr1el’s picture

Gabr1el CreditAttribution: Gabr1el for ASSIST Software SRL commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.05 KB
2696103-4.patch1.38 KB

I fixed the failure reason, interdiff

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Let's add a test for this. Would fit nicely in a KernelTest - perhaps we should add a new one in Drupal\KernelTests\Core\Config for this.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

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.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

chOP’s picture

chOP CreditAttribution: chOP commented
Version: 8.6.x-dev » 8.7.x-dev
Status: Needs work » Needs review
Issue tags: -Needs tests +DrupalSouth 2018
FileSize
2696103-11.patch2.39 KB

I've re-rolled this and updated the tests. This new exception now has a test for it, so removing the tag.

Completed during DrupalSouth 2018 code sprint.

Status: Needs review » Needs work

The last submitted patch, 11: 2696103-11.patch, failed testing. View results

chOP’s picture

chOP CreditAttribution: chOP at Deloitte Digital commented
Status: Needs work » Needs review

Re-submitted with version 8.7.x-dev and tests passed.
Failed testing when applied to 8.6.x-dev which is expected.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
CreditAttribution: voleger as a volunteer and at GOLEMS GABB, Drupal Ukraine Community for Drupal Ukraine Community, GOLEMS GABB commented

This issue is outdated as the config_get_config_directory() function going to be removed from drupal:9.0.0

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Title: config_get_config_directory() should throw a more specific exception » \Drupal\Core\Config\FileStorageFactory::getSync() should throw a more specific exception
Status: Needs review » Needs work

Updated title and status

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented
Issue tags: +Needs reroll
ashok.gharpankar’s picture

ashok.gharpankar CreditAttribution: ashok.gharpankar as a volunteer and at Trigyn Technologies Ltd commented
Assigned: Unassigned » ashok.gharpankar
ashok.gharpankar’s picture

ashok.gharpankar CreditAttribution: ashok.gharpankar as a volunteer and at Trigyn Technologies Ltd commented
Status: Needs work » Active
ashok.gharpankar’s picture

ashok.gharpankar CreditAttribution: ashok.gharpankar as a volunteer and at Trigyn Technologies Ltd for Drupal India Association commented
Issue tags: +Trigyn-WeeklyCodeSprint
FileSize
2696103-22.patch1.69 KB
ashok.gharpankar’s picture

ashok.gharpankar CreditAttribution: ashok.gharpankar as a volunteer and at Trigyn Technologies Ltd for Drupal India Association commented
Assigned: ashok.gharpankar » Unassigned
Status: Active » Needs review
DerekCresswell’s picture

DerekCresswell CreditAttribution: DerekCresswell as a volunteer and at Acro Commerce commented
Category: Bug report » Task
FileSize
config-2696103-24.patch1.58 KB

I've changed the name of the exception based on number 5 of the list here in the coding standards.

From InvalidConfigDirectoryTypeException to ConfigDirectoryNotDefinedException as the subsystem is supposed to come before the error ([Subsystem][ErrorType]Exception).

DerekCresswell’s picture

DerekCresswell CreditAttribution: DerekCresswell as a volunteer and at Acro Commerce commented
Issue tags: -DrupalSouth 2018, -Needs reroll, -Trigyn-WeeklyCodeSprint
DerekCresswell’s picture

DerekCresswell CreditAttribution: DerekCresswell as a volunteer and at Acro Commerce commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Needs work

Last patch has no tests, which were added in #11

DerekCresswell’s picture

DerekCresswell CreditAttribution: DerekCresswell as a volunteer and at Acro Commerce commented
Status: Needs work » Needs review
FileSize
config-2696103-28.patch2.54 KB

Added back the testing of the new exception.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

The patch looks good to me.
The new and more specific exception is being tested.
As stated in the IS, the change does not break BC.
Added a CR.
For me the issue is RTBC.

alexpott’s picture

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

Committed 474b03c and pushed to 9.1.x. Thanks!

diff --git a/core/tests/Drupal/KernelTests/Core/Config/FileStorageFactoryTest.php b/core/tests/Drupal/KernelTests/Core/Config/FileStorageFactoryTest.php
index 7bf12ae1cc..4dd364754c 100644
--- a/core/tests/Drupal/KernelTests/Core/Config/FileStorageFactoryTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Config/FileStorageFactoryTest.php
@@ -35,13 +35,9 @@ public function testGetSync() {
     new Settings($settings);
 
     // On an empty settings there is an exception thrown.
-    try {
-      FileStorageFactory::getSync();
-      $this->fail("The exception was not thrown.");
-    }
-    catch (ConfigDirectoryNotDefinedException $exception) {
-      $this->assertEquals('The config sync directory is not defined in $settings["config_sync_directory"]', $exception->getMessage());
-    }
+    $this->expectException(ConfigDirectoryNotDefinedException::class);
+    $this->expectExceptionMessage('The config sync directory is not defined in $settings["config_sync_directory"]');
+    FileStorageFactory::getSync();
   }
 
 }

Improved the test to use a more modern PHPUnit pattern.

  • alexpott committed 474b03c on 9.1.x 
    Issue #2696103 by Gabr1el, DerekCresswell, ashok.gharpankar, chOP,...

Status: Fixed » Closed (fixed)

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