Problem

  • The namespace part in a configuration object is essential for Drupal core to maintain modules + config objects, but the config system allows to save a config object without a namespace (e.g., just $module).
  • The configuration file name needs to be fewer than 255 characters, implying an object name of 250 characters or less, but the Configuration system does not enforce this requirement. (See #1186034: Length of configuration object name can be too small.)

Proposed solution

  • Add a ConfigObjectNameException for invalid object names.
  • Validate configuration object names to reject names which do not contain at least one dot/period (".") or is over 250 characters.
  • Perform validation on save, synch, and import operations to ensure that invalid objects are not created or imported from other sources.
Files: 
CommentFileSizeAuthor
#81 config-1701014-81.patch9.25 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 49,659 pass(es). View
#81 interdiff.txt3.43 KBxjm
#75 config-1701014-75.patch9.05 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,525 pass(es). View
#75 interdiff.txt4.43 KBxjm
#73 config-1701014-73.patch6.97 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,494 pass(es). View
#73 interdiff.txt3.71 KBxjm
#70 config.validate-name.70.patch8.42 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,375 pass(es), 41 fail(s), and 1 exception(s). View
#65 1701014-verify-config-names-65.patch6.31 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 48,798 pass(es), 1 fail(s), and 6 exception(s). View
#65 interdiff-61-65.txt625 bytesheyrocker
#61 config-1701014-61-tests.patch2.46 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 48,870 pass(es), 1 fail(s), and 2 exception(s). View
#61 config-1701014-61.patch6.31 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#61 interdiff.txt996 bytesxjm
#58 config-1701014-58-tests.patch2.46 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.php. View
#58 config-1701014-58.patch6.31 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.php. View
#58 interdiff.txt6.79 KBxjm
#49 drupal-1701014-config_save_namespace-49.patch3.6 KBboztek
PASSED: [[SimpleTest]]: [MySQL] 42,094 pass(es). View
#44 drupal-1701014-config_save_namespace-44.patch3.64 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1701014-config_save_namespace-44.patch. Unable to apply patch. See the log in the details link for more information. View
#41 drupal-1701014-config_save_namespace-41.patch246.75 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1701014-config_save_namespace-41.patch. Unable to apply patch. See the log in the details link for more information. View
#41 interdiff.txt2.5 KBdisasm
#39 drupal-1701014-config_save_namespace-39.patch3.48 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 40,244 pass(es). View
#39 interdiff.txt1.13 KBdisasm
#36 drupal-1701014-config_save_namespace-36.patch3.71 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 40,251 pass(es), 1 fail(s), and 3 exception(s). View
#36 interdiff.txt3.75 KBdisasm
#33 interdiff.txt1.36 KBdisasm
#32 drupal-1701014-config_save_namespace-32.patch4.26 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 39,994 pass(es). View
#29 drupal-1701014-config_save_namespace-29-test-only.patch1.3 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 39,989 pass(es), 1 fail(s), and 0 exception(s). View
#29 drupal-1701014-config_save_namespace-29.patch4.3 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 39,990 pass(es). View
#29 interdiff.txt3.49 KBdisasm
#26 drupal-1701014-config_save_namespace-26.patch4.84 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 39,698 pass(es). View
#26 interdiff.txt2.25 KBdisasm
#24 drupal-1701014-config_save_namespace-24-test-only.patch1.76 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 39,985 pass(es), 1 fail(s), and 0 exception(s). View
#24 drupal-1701014-config_save_namespace-24.patch4.81 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 39,994 pass(es). View
#21 drupal-1701014-config_save_namespace-21-test-only.patch1.76 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 39,997 pass(es), 1 fail(s), and 0 exception(s). View
#21 drupal-1701014-config_save_namespace-21.patch4.8 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 39,994 pass(es). View
#17 drupal-1701014-config_save_namespace-17-test-only.patch1.75 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 39,985 pass(es), 1 fail(s), and 0 exception(s). View
#17 drupal-1701014-config_save_namespace-17.patch4.8 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 39,986 pass(es). View
#15 drupal-1701014-config_save_namespace-15-test-only.patch1.75 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 39,993 pass(es), 1 fail(s), and 0 exception(s). View
#15 drupal-1701014-config_save_namespace-15.patch4.79 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#14 drupal8.config-save-namespace.14.patch3.05 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,768 pass(es). View
#1 drupal8.config-save-namespace.1.patch831 bytessun
PASSED: [[SimpleTest]]: [MySQL] 37,212 pass(es). View

Comments

sun’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
831 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,212 pass(es). View

Status: Needs review » Needs work

The last submitted patch, drupal8.config-save-namespace.1.patch, failed testing.

sun’s picture

err, what? Do we have malformed config object names in core already...?

sun’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Configuration system

Status: Needs review » Needs work

The last submitted patch, drupal8.config-save-namespace.1.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +Configuration system
sun’s picture

I think this patch is actually RTBC.

The exception is not caught anywhere, but that's intended. The system should fail prominently if someone is trying to save a config object without a proper name including namespace.

heyrocker’s picture

Do we want to check when a module is installed to verify that it hasn't supplied an improperly named file as well? Possibly the same thing on import?

sun’s picture

Well, this patch sorta is a catch-all.

The check is injected into the low-level Config::save() method, so it applies to everything; config(), module installation, import, etc.pp. If anything at any point in time is trying to save config with a bogus config object name, the system blows up.

heyrocker’s picture

Right but so far as I can see, default module installation doesn't actually call Config::save(). It calls config_sync_changes() which does direct read/write operations on the storage objects themselves.

sun’s picture

Hm. You're right.

Though then again, in order to have any config to import, that config must have been saved at some point, and at that point, this validation would be triggered.

The alternative would be to implant a similar check somewhere into the config_import* functions (though I'm not sure which location would be appropriate currently).

heyrocker’s picture

That's not necessarily true, any module author can create any default config file by hand they want. I'm sure many people will make a 'settings.yml' at some point and expect it work. It certainly won't work as expected, its a bug ultimately. I'm not sure this is worth protecting against or not, but I wanted to think through all the places this might be needed.

I don't really know where the best place to protect against this would be either. My first inclination was to build it into the FileStorage object, but then we're putting business logic in the storage object and that is no good.

catch’s picture

Title: Config system does not prevent to save a config object without namespace » Config system does not prevent saving a config object without namespace

It calls config_sync_changes() which does direct read/write operations on the storage objects themselves.

If the eventual storage dispatcher just contains a list of storage objects to read and/or write to, then won't we eventually be able to use Config::save() there too?

sun’s picture

FileSize
3.05 KB
PASSED: [[SimpleTest]]: [MySQL] 39,768 pass(es). View

We don't want to rely on StorageDispatcher, since we'll #1671080: Remove StorageDispatcher to simplify configuration system

So I suggest to just simply implant the identical check into the config import functions.

disasm’s picture

FileSize
4.79 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.75 KB
FAILED: [[SimpleTest]]: [MySQL] 39,993 pass(es), 1 fail(s), and 0 exception(s). View

attached is a test for saving configs without a namespace.

Status: Needs review » Needs work

The last submitted patch, drupal-1701014-config_save_namespace-15.patch, failed testing.

disasm’s picture

FileSize
4.8 KB
PASSED: [[SimpleTest]]: [MySQL] 39,986 pass(es). View
1.75 KB
FAILED: [[SimpleTest]]: [MySQL] 39,985 pass(es), 1 fail(s), and 0 exception(s). View

uploaded the wrong files. here's the correct ones.

disasm’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Status: Needs review » Needs work

The test makes sense, here's some stuff to clean up.

+++ b/core/includes/config.incundefined
@@ -113,10 +115,16 @@ function config_sync_get_changes(StorageInterface $source_storage, StorageInterf
+        throw new ConfigException(sprintf('Missing namespace in Config name %s', $name));

Why use sprintf? Why not format_string?

+++ b/core/includes/config.incundefined
@@ -188,6 +198,10 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
+        throw new ConfigException(sprintf('Missing namespace in Config name %s', $name));

Same here.

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -238,8 +238,15 @@ class Config {
+      throw new ConfigException(sprintf('Missing namespace in Config name %s', $this->name));

Once again.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigNamespaceTest.phpundefined
@@ -0,0 +1,55 @@
+class ConfigNamespaceTest extends WebTestBase {
+  public static function getInfo() {

There should be a blank line between class and getInfo

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigNamespaceTest.phpundefined
@@ -0,0 +1,55 @@
+      if($e->getMessage() == "Missing namespace in Config name nonamespace") {

Needs a space after 'if' before '('

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigNamespaceTest.phpundefined
@@ -0,0 +1,55 @@
+    $this->assertTrue($caught_exception,'No Namespace exception caught when namespace not specified in config name');
+    $name = 'config.namespace';

Add a blank line between these lines

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigNamespaceTest.phpundefined
@@ -0,0 +1,55 @@
+      if($e->getMessage() == "Missing namespace in Config name nonamespace") {

Needs a space after 'if' before '('

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigNamespaceTest.phpundefined
@@ -0,0 +1,55 @@
+    $this->assertFalse($caught_exception,'No Namespace exception not caught when namespace exists in config name');
+  }

And another blank line after the method before the end of the class

sun’s picture

We're using sprintf() in exceptions to decouple the Config system from procedural code in Drupal core. One of the final goals is to move it into Drupal\Component.

disasm’s picture

Status: Needs work » Needs review
FileSize
4.8 KB
PASSED: [[SimpleTest]]: [MySQL] 39,994 pass(es). View
1.76 KB
FAILED: [[SimpleTest]]: [MySQL] 39,997 pass(es), 1 fail(s), and 0 exception(s). View

attached are patches with Tim's changes.

tim.plunkett’s picture

@sun, that makes sense. Is there an issue? It needs to be documented on http://drupal.org/node/608166

Cottser’s picture

disasm’s picture

FileSize
4.81 KB
PASSED: [[SimpleTest]]: [MySQL] 39,994 pass(es). View
1.76 KB
FAILED: [[SimpleTest]]: [MySQL] 39,985 pass(es), 1 fail(s), and 0 exception(s). View

After viewing document Tim linked above, caught to more style issues, this time with catch not having a ' ' after it.

Attached are more patches.

Cottser’s picture

Thanks @disasm!

A few other minor things I noticed:

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigNamespaceTest.phpundefined
@@ -0,0 +1,58 @@
+ * Tests saving config without namespace

This comment should end with a period.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigNamespaceTest.phpundefined
@@ -0,0 +1,58 @@
+      if ($e->getMessage() == "Missing namespace in Config name nonamespace") {
...
+      if ($e->getMessage() == "Missing namespace in Config name nonamespace") {

These could be single quotes.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigNamespaceTest.phpundefined
@@ -0,0 +1,58 @@
+    $this->assertTrue($caught_exception,'No Namespace exception caught when namespace not specified in config name');
...
+    $this->assertFalse($caught_exception,'No Namespace exception not caught when namespace exists in config name');

There should be a space after the first argument and before 'No Namespace…'

The consensus seems to be that the assertion messages should end in a period.

The test only patch in #21 showed 0 passes but it looks like that's #1717868: PIFR uses variable_get() but needs config() in Drupal 8.

disasm’s picture

FileSize
2.25 KB
4.84 KB
PASSED: [[SimpleTest]]: [MySQL] 39,698 pass(es). View

attached is a patch and interdiff to 24

Cottser’s picture

We reviewed this in IRC, thanks again @disasm.

One other minor point that was discussed after reviewing #26 - the 'description' in getInfo() could be a bit broader, rather than describing the testNamespace() method.

sun’s picture

Status: Needs review » Needs work

Thanks for moving this forward! :)

Almost done! Some last issues:

+++ b/core/includes/config.inc
@@ -1,6 +1,8 @@
+use Drupal\Core\Config\DatabaseStorage;

No longer used in this file and can be removed here.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigNamespaceTest.php
@@ -0,0 +1,58 @@
+ * Definition of Drupal\config\Tests\ConfigNamespaceTest.
...
+  function testNamespace() {

I wonder whether we really need an entirely new test case class. Looks like we could just add the new test method to ConfigCRUDTest instead?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigNamespaceTest.php
@@ -0,0 +1,58 @@
+use Drupal\Core\Config\DatabaseStorage;

Doesn't seem to be used in this file, so can be removed.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigNamespaceTest.php
@@ -0,0 +1,58 @@
+    $caught_exception = FALSE;
+    try {
+      $config = config($name);
+      $config->save();
+    }
+    catch (ConfigException $e) {
+      if ($e->getMessage() == 'Missing namespace in Config name nonamespace') {
+        $caught_exception = TRUE;
+      }
+    }
+    $this->assertTrue($caught_exception, 'No Namespace exception caught when namespace not specified in config name.');

We don't need to assert the exact exception message.

The common way to test exceptions is:

try {
  config($name)->save();
  $this->fail('ConfigException was thrown.');
}
catch (ConfigException $e) {
  $this->pass($e . ' was thrown.');
}
disasm’s picture

FileSize
3.49 KB
4.3 KB
PASSED: [[SimpleTest]]: [MySQL] 39,990 pass(es). View
1.3 KB
FAILED: [[SimpleTest]]: [MySQL] 39,989 pass(es), 1 fail(s), and 0 exception(s). View

Since I moved the test and changed the functionality of the pass/fail logic, I've attached the test-only as well this time. Full inventory:
test-only-patch (should fail)
patch (should pass)
interdiff to patch in 26.

disasm’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Needs work

Thanks! Almost done!

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.php
@@ -112,4 +113,28 @@ class ConfigCRUDTest extends WebTestBase {
   }
+  /**

Missing newline before new phpDoc block.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.php
@@ -112,4 +113,28 @@ class ConfigCRUDTest extends WebTestBase {
+      $this->fail('Expected exception ConfigException was not thrown.');
...
+      $this->pass('Expected exception ConfigException was thrown.');
...
+      $this->pass('Exception ConfigException was not thrown.');
...
+      $this->fail('Exception ConfigException was thrown.');

Let's remove the word "exception" in all of the messages (leaving only ConfigException).

disasm’s picture

Status: Needs work » Needs review
FileSize
4.26 KB
PASSED: [[SimpleTest]]: [MySQL] 39,994 pass(es). View

attached is patch fixing comments from 31.

disasm’s picture

FileSize
1.36 KB

forgot the interdiff...

sun’s picture

Status: Needs review » Reviewed & tested by the community

Excellent, thanks! :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

Why are we copy/pasting the same code three times? Isn't there a single place this can be checked? Or a helper if not?

I've complained about sprintf() in Symfony method overrides where it was copied from there, and I don't like it being introduced here either. We already have format_string() and t() that do placeholder substitution in core, and encourage people to use those for translation, avoiding security mis-haps etc.. If we start using sprintf() in seemingly arbitrary places (even if it's just exceptions, but I'm sure there'll be other places eventually), then it's going to confuse that issue - likely in the direction of people just using raw sprintf() all over the place. Since this code doesn't have the get-out of being a copy/paste from Symfony that's not even executed and ought to be removed again at some point, I'm not going to let it go in like this. Let's use format_string() here, then open a separate issue to discuss just using sprintf() in Drupal components to avoid the procedural dependency, since it's got nothing to do with this issue at all.

disasm’s picture

FileSize
3.75 KB
3.71 KB
FAILED: [[SimpleTest]]: [MySQL] 40,251 pass(es), 1 fail(s), and 3 exception(s). View

Attached is a patch that moves copy/pasted code into static function Config::checkNamespace($name).

Also, patch uses format_string as catch requested above.

Interdiff to #32 included.

disasm’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/includes/config.incundefined
@@ -1,6 +1,7 @@
+use Drupal\Core\Config\Config\ConfigException;

This isn't needed (and has too many '\Config's anyway)

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -288,8 +289,12 @@ class Config {
    * Saves the configuration object.
+   *

No need for the extra line.

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -326,4 +331,15 @@ class Config {
+  public static function checkNamespace($name)
+  {
+    if (strpos($name, '.') === FALSE) {

The opening curly brace should be on the same line

disasm’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
3.48 KB
PASSED: [[SimpleTest]]: [MySQL] 40,244 pass(es). View

attached is patch with requested changes.
It looks like the failed test above was unrelated to this patch, but we'll see what happens when this one is tested.
Interdiff to #36.

sun’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Thanks! Almost done :)

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -7,6 +7,7 @@
+use Drupal\Core\Config\ConfigException;
 use Drupal\Component\Utility\NestedArray;

Let's order these alphabetically.

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -326,4 +330,14 @@ class Config {
   }
+  /**

Missing newline between methods.

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -326,4 +330,14 @@ class Config {
+   * Checks if a configuration object is namespaced by extension.
+   *
+   * @throws Drupal\Core\Config\ConfigException
+   */
+  public static function checkNamespace($name) {

What do you think of renaming this to validateNamespace() and adjusting the phpDoc accordingly?

disasm’s picture

FileSize
2.5 KB
246.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1701014-config_save_namespace-41.patch. Unable to apply patch. See the log in the details link for more information. View

attached are requested changes

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-1701014-config_save_namespace-41.patch, failed testing.

disasm’s picture

FileSize
3.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1701014-config_save_namespace-44.patch. Unable to apply patch. See the log in the details link for more information. View

patch failed to apply, here's a reroll.

disasm’s picture

Status: Needs work » Needs review
sun’s picture

sun’s picture

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, drupal-1701014-config_save_namespace-44.patch, failed testing.

boztek’s picture

Status: Needs work » Needs review
FileSize
3.6 KB
PASSED: [[SimpleTest]]: [MySQL] 42,094 pass(es). View

Rerolled.

leschekfm’s picture

The patch looks fine for me.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Excellent, thanks! :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

Checking this in three different places still feels like too much - what's wrong with just a non-static function and checking in save()?

sun’s picture

As visible in the diff, it is not checked in save() only.

catch’s picture

Yes I know that. Why can't we only check it in save?

sun’s picture

+++ b/core/includes/config.inc
@@ -116,6 +116,7 @@ function config_sync_get_changes(StorageInterface $source_storage, StorageInterf
 function config_sync_changes(array $config_changes, StorageInterface $source_storage, StorageInterface $target_storage) {
   foreach (array('delete', 'create', 'change') as $op) {
     foreach ($config_changes[$op] as $name) {
+      Config::validateNamespace($name);
       if ($op == 'delete') {
         $target_storage->delete($name);

Because this does not go through save() — not even through a Config object at all?

xjm’s picture

Title: Config system does not prevent saving a config object without namespace » Validate config object names
Assigned: Unassigned » xjm
xjm’s picture

I updated the issue summary to reflect the current scope and answer @catch's question. We want to validate the names whenever we have new configuration, which means it needs to happen both on save and on synch. Fail early, fail often, fail well.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Status: Needs work » Needs review
FileSize
6.79 KB
6.31 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.php. View
2.46 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.php. View

Attached merges in the change and test coverage from #1186034-16: Length of configuration object name can be too small.

Note that #52 raises a point: right now, the tests only assert the expected behavior on save() operations. If we want to make sure the validation also happens on synch and import, we should add test coverage for that. I haven't added that yet in this patch.

Status: Needs review » Needs work

The last submitted patch, config-1701014-58.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
996 bytes
6.31 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
2.46 KB
FAILED: [[SimpleTest]]: [MySQL] 48,870 pass(es), 1 fail(s), and 2 exception(s). View

Oops.

Status: Needs review » Needs work

The last submitted patch, config-1701014-61.patch, failed testing.

chx’s picture

While at it, could we please exclude : ? * < > " | / \ characters as well?

heyrocker’s picture

One other thing we talked about in #1479454-82: Convert user roles to configurables is adding a trim() to all names.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
625 bytes
6.31 KB
FAILED: [[SimpleTest]]: [MySQL] 48,798 pass(es), 1 fail(s), and 6 exception(s). View

This at least gets through the installer. Haven't gone much further but figured I'd push it up.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -347,6 +360,8 @@ public function load() {
+    $this->validateName($this->name);

I think this should technically be static::validateName($this->name);, but I don't know if it matters

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -416,4 +431,34 @@ public function merge(array $data_to_merge) {
+    if (strlen($name) > self::MAX_NAME_LENGTH) {

The self:: should be static:::

Status: Needs review » Needs work

The last submitted patch, 1701014-verify-config-names-65.patch, failed testing.

heyrocker’s picture

Status: Needs work » Needs review

These fails seem to be from the following line in rest.module

$resources = config('rest')->get('resources');

This naming is obviously broken, and rest.module also provides no default config. #1858676: Rest module config is named improperly and no default config is supplied. has been opened to address this.

xjm’s picture

I've downloaded this patch and issue to take a look at offline.

sun’s picture

FileSize
8.42 KB
FAILED: [[SimpleTest]]: [MySQL] 49,375 pass(es), 41 fail(s), and 1 exception(s). View

- Incorporated #1858676: Rest module config is named improperly and no default config is supplied..
- Moved constant definition in Config first (constants should always defined first).
- Fixed coding style in new validateName() method.
- Fixed test tries to catch unknown exception.

Status: Needs review » Needs work

The last submitted patch, config.validate-name.70.patch, failed testing.

chx’s picture

+ // Validate the configuration object name before saving.
+ $this->validateName($this->name);

This should be static:: not $this->

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
FileSize
3.71 KB
6.97 KB
PASSED: [[SimpleTest]]: [MySQL] 50,494 pass(es). View

Fixed coding style in new validateName() method

I disagree; your "fix" is not related to any coding standard that has consensus, but I don't care enough to change it. ;) People complain about both formats and it's really a matter of preference.

Anyway, the attached:

Still to do: confirm that the exception gets thrown properly on import and synch (one assertion each should be sufficient).

damiankloip’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -427,7 +435,7 @@ public function load() {
+    Config::validateName($this->name);

I don't think this should be changed to the class name? Shouldn't this be static::validateName() instead?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined
@@ -132,8 +132,24 @@ function testNameValidation() {
+        $config = config($name);
+        $config->save();

Small point, but the $config variable doesn't seem to be used. So this could just be config($name)->save(); on one line.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined
@@ -132,8 +132,24 @@ function testNameValidation() {
+    $this->assertTrue(empty($test_characters), format_string('ConfigNameException was thrown for all invalid name characters: @characters', array(
+      '@characters' => implode(' ', $characters),
+    )));

Nice, I like the idea of this. I'll remember that when I need to test exceptions :)

Also, I opened #1879774: Catch plugin exceptions for invalid views display plugins as we want to deal with display plugin errors instead I think. So we will want to change, or just remove the views validation in this patch maybe.

xjm’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
9.05 KB
PASSED: [[SimpleTest]]: [MySQL] 50,525 pass(es). View

I don't think this should be changed to the class name? Shouldn't this be static::validateName() instead?

Isn't this the same thing? I guess if someone wanted to subclass Config and override the method...? I changed it in any case.

Small point, but the $config variable doesn't seem to be used. So this could just be config($name)->save(); on one line.

OK.

I also added the two additional assertions per #73. In the process of so doing I noticed that config_sync_changes() is not ever called in core without config_import_invoke_owner() being called first; do we still need to validate the name in both?

damiankloip’s picture

Isn't this the same thing? I guess if someone wanted to subclass Config and override the method...? I changed it in any case.

No, not the same thing, it's not internal to the class then, as it's hardcoding the Config class. I don't think we do this anywhere else (or hopefully not!) That's how we would call this from outside the class. So I think it kind of has to be changed ;)

Patch is looking good.

xjm’s picture

+++ b/core/includes/config.incundefined
@@ -182,6 +182,8 @@ function config_sync_changes(array $config_changes, StorageInterface $source_sto
   $factory = drupal_container()->get('config.factory');
   foreach (array('delete', 'create', 'change') as $op) {
     foreach ($config_changes[$op] as $name) {
+      // Validate the configuration object name before importing it.
+      Config::validateName($name);
       if ($op == 'delete') {
         $target_storage->delete($name);
       }
@@ -256,6 +258,8 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou

@@ -256,6 +258,8 @@ function config_import_invoke_owner(array $config_changes, StorageInterface $sou
       // Call to the configuration entity's storage controller to handle the
       // configuration change.
       $handled_by_module = FALSE;
+      // Validate the configuration object name before importing it.
+      Config::validateName($name);
       if ($entity_type = config_get_entity_type_by_name($name)) {
         $old_config = new Config($name, $target_storage);
         $old_config->load();

Do we also need to change these, then?

xjm’s picture

Okay, apparently #77 isn't a problem and I just don't understand things very well. I had to re-read #76. It seems a little weird to have some procedural and some OO code around this, but that isn't in scope for this issue.

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -16,6 +17,18 @@
+   * The maximum length of a configuration object name.
+   *
+   * Many filesystems (including HFS, NTFS, and ext4) have a maximum file name
+   * length of 255 characters. To ensure that no configuration objects
+   * incompatible with this limitation are created, we enforce a maximum name
+   * length of 250 characters (leaving 5 characters for the file extension).
+   *
+   * @see http://en.wikipedia.org/wiki/Comparison_of_file_systems
+   */
+  const MAX_NAME_LENGTH = 250;

It's really nice seeing this documented with references now.

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -123,6 +136,37 @@ public function setName($name) {
+    // The name must be namespaced by owner.
+    if (strpos($name, '.') === FALSE) {
+      throw new ConfigNameException(format_string('Missing namespace in Config object name @name.', array(
+        '@name' => $name,
+      )));
+    }

We will probably need a followup to make sure that not only do we have a namespace, but that it is an installed module/theme, because that will likely become codified when #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API is sorted out.

I'm happy, thanks all!

sun’s picture

This patch looks also great to me.

Not sure on disallowing forward-slashes yet, since config context was supposed to use subdirectories for overrides, which might need to be included in the name in some form, but for now, it's absolutely better to disallow everything, so let's definitely move forward with this patch!

Thanks all!

xjm’s picture

FileSize
3.43 KB
9.25 KB
PASSED: [[SimpleTest]]: [MySQL] 49,659 pass(es). View

@webchick asked for clarification about how the filenames in the last two assertions were invalid, and my discussion made it apparent that the naming and documentation of the test module was unclear, so the attached patch makes the names more self-explanatory and adds docs.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks great, thanks!

Committed and pushed to 8.x.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes
Issue tags: +Naming things is hard