The Config object uses Drupal\Component\Utility\NestedArray to retrieve nested values using dotted strings. The stub provided by the Drupal\Test\UnitTestCase doesn't do this, and will only handle keys in the top level.

Basically, this fails:

  $config = $this->getConfigFactoryStub(['name' => ['a' => ['b' => 'c']]]);
  $this->assertEquals('c', $config->get('name')->get('a.b'));

Fixing this will probably require a rewrite of the method, as it will have to use callbacks instead of return value maps - unless we traverse the entire array and copy nested values into the top level of the return value map.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cburschka created an issue. See original summary.

cburschka’s picture

Issue summary: View changes
cburschka’s picture

Issue summary: View changes
cburschka’s picture

A work-around for tests using the latter approach is this:

  /**
   * {@inheritdoc}
   */
  public function getConfigFactoryStub(array $configs = []) {
    foreach ($configs as $id => $config) {
      $configs[$id] = self::flatten($config);
    }
    return parent::getConfigFactoryStub($configs);
  }

  /**
   * Flatten nested arrays into the top level as dotted keys.
   *
   * Original keys remain intact.
   *
   * @param array  $config
   * @param string $prefix
   *
   * @return array
   */
  private static function flatten(array $config, $prefix = '') {
    $flat = [];
    foreach ($config as $name => $value) {
      $flat[$prefix . $name] = $value;
      if (is_array($value)) {
        $flat += self::flatten($value, $prefix . $name . '.');
      }
    }
    return $flat;
  }

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.

m4olivei’s picture

Thanks @cburschka for the workaround, working great for us. As we've now used the workaround in a couple places, I'm going to take a stab at resolving it in Drupal core.

First crack at a patch attached.

m4olivei’s picture

Status: Active » Needs review

Status: Needs review » Needs work
m4olivei’s picture

The last test run had failures that didn't make a lot of sense. Re-running.

cburschka’s picture

The failing tests seem to be ones which are written with this in mind:

$this->configFactory = $this->getConfigFactoryStub(['system.site' => ['page.403' => '/access-denied-page', 'page.404' => '/not-found-page']]);

In order to be backward-compatible, the patch will need to look for dotted strings in the given data, and convert it into a nested array.

m4olivei’s picture

Ahh, I totally missed that thanks for pointing that out. I'll work on that.

m4olivei’s picture

Here's a second attempt, this will check if we have the requested dot syntax key as is in the $configs array passed to getConfigFactoryStub before moving on to explode the key and look using NestedArray.

m4olivei’s picture

Yay, it passed!

deviantintegral’s picture

Version: 8.4.x-dev » 8.5.x-dev
Assigned: m4olivei » Unassigned
Status: Needs review » Reviewed & tested by the community
deviantintegral’s picture

Status: Reviewed & tested by the community » Needs review
deviantintegral’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies and passes against 8.5.x. Code looks good to me!

Wim Leers’s picture

Title: UnitTestCase::getConfigFactoryStub doesn't accept dotted keys » UnitTestCase::getConfigFactoryStub() doesn't accept dotted keys
Issue tags: +Contributed project soft blocker

Hah! While scanning the RTBC queue, this one caught my eye.

I also ran into this in #2708787, in the CDN contrib module that I maintain. I added a work-around in #2708787-5: Port Far Future expiration to 8.x-3.x, but then forgot to report it back to the Drupal core issue queue.

I overrode UnitTestCase::getConfigFactoryStub() in my unit test, with the following interdiff:

diff --git a/core/tests/Drupal/Tests/UnitTestCase.php b/core/tests/Drupal/Tests/UnitTestCase.php
index 58173e5..030b5e4 100644
--- a/core/tests/Drupal/Tests/UnitTestCase.php
+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -111,20 +111,24 @@ public function getConfigFactoryStub(array $configs = []) {
     $config_editable_map = [];
     // Construct the desired configuration object stubs, each with its own
     // desired return map.
-    foreach ($configs as $config_name => $config_values) {
-      $map = [];
-      foreach ($config_values as $key => $value) {
-        $map[] = [$key, $value];
-      }
-      // Also allow to pass in no argument.
-      $map[] = ['', $config_values];
+    foreach ($configs as $config_name => $map) {
+      $get = function ($key) use ($map) {
+        $parts = explode('.', $key);
+        if (count($parts) == 1) {
+          return isset($map[$key]) ? $map[$key] : NULL;
+        }
+        else {
+          $value = NestedArray::getValue($map, $parts, $key_exists);
+          return $key_exists ? $value : NULL;
+        }
+      };
 
       $immutable_config_object = $this->getMockBuilder('Drupal\Core\Config\ImmutableConfig')
         ->disableOriginalConstructor()
         ->getMock();
       $immutable_config_object->expects($this->any())
         ->method('get')
-        ->will($this->returnValueMap($map));
+        ->willReturnCallback($get);
       $config_get_map[] = [$config_name, $immutable_config_object];
 
       $mutable_config_object = $this->getMockBuilder('Drupal\Core\Config\Config')
@@ -132,7 +136,7 @@ public function getConfigFactoryStub(array $configs = []) {
         ->getMock();
       $mutable_config_object->expects($this->any())
         ->method('get')
-        ->will($this->returnValueMap($map));
+        ->willReturnCallback($get);
       $config_editable_map[] = [$config_name, $mutable_config_object];
     }
     // Construct a config factory with the array of configuration object stubs

And had this docblock for my override:

  /**
   * {@inheritdoc}
   *
   * Overridden, because the way ImmutableConfig::get() is mocked, does not
   * match the actual implementation, which then causes tests to fail.
   */

Glad to see this being solved in Drupal core! 🎉

Wim Leers’s picture

Wim Leers’s picture

Conclusion: this is ready!

Wim Leers’s picture

Finally: this totally also belongs as a bugfix in 8.4.

larowlan’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Needs review
+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -111,19 +113,29 @@ public function getConfigFactoryStub(array $configs = []) {
+        if ($key == '') {

Is 0 a valid key? If so, this should use ===

See https://3v4l.org/3V8ca

Wim Leers’s picture

Status: Needs review » Needs work

I … am not sure. But \Drupal\Core\Config\Config::get() uses if (empty($key)), so let's just use that instead: then the mock behavior matches the implementation behavior.

m4olivei’s picture

Nice catch, empty() check sounds good. Here's an updated patch.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
larowlan’s picture

So that answers the question then 0 is not a valid key - because empty() would be TRUE.

Nice to know - thanks.

Adding review credits for those who shaped the patch

  • larowlan committed bcb5afd on 8.5.x
    Issue #2862248 by m4olivei, Wim Leers, cburschka, larowlan: UnitTestCase...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as bcb5afd and pushed to 8.5.x

Wim Leers’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Fixed » Reviewed & tested by the community

Quoting @m4olivei from #6:

Thanks @cburschka for the workaround, working great for us. As we've now used the workaround in a couple places, I'm going to take a stab at resolving it in Drupal core.

This is a problem on existing Drupal sites. Don't we want to commit this to 8.4 as well?

Status: Reviewed & tested by the community » Needs work
cburschka’s picture

Status: Needs work » Needs review

Testing again for 8.4.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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.

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

8.4.x is critical fixes only now, this is already in 8.5.x - marking closed

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Drupal 8.5.0 shipped earlier today, so I was able to commit #2934643: Remove \Drupal\Tests\cdn\Unit\File\FileUrlGeneratorTest::getConfigFactoryStub() once the CDN module can require Drupal 8.5 at last :) I also credited everyone on this issue. Because it's thanks to all of you that core is again a little bit better, and contrib needs to work around fewer things! ❤️