Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff.txt | 621 bytes | m4olivei |
#23 | allow_dotted_names_for_config_factory_stubs-2862248-23.patch | 3.69 KB | m4olivei |
#12 | allow_dotted_names_for_config_factory_stubs-2862248-12.patch | 3.69 KB | m4olivei |
#6 | allow_dotted_names_for_config_factory_stubs-2862248-6.patch | 3.71 KB | m4olivei |
Comments
Comment #2
cburschkaComment #3
cburschkaComment #4
cburschkaA work-around for tests using the latter approach is this:
Comment #6
m4oliveiThanks @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.
Comment #7
m4oliveiComment #9
m4oliveiThe last test run had failures that didn't make a lot of sense. Re-running.
Comment #10
cburschkaThe 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.
Comment #11
m4oliveiAhh, I totally missed that thanks for pointing that out. I'll work on that.
Comment #12
m4oliveiHere'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.
Comment #13
m4oliveiYay, it passed!
Comment #14
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedComment #15
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedComment #16
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedPatch applies and passes against 8.5.x. Code looks good to me!
Comment #17
Wim LeersHah! 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:And had this docblock for my override:
Glad to see this being solved in Drupal core! 🎉
Comment #18
Wim LeersComment #19
Wim Leers\Drupal\Core\Config\Config::get()
.Hurray!
Conclusion: this is ready!
Comment #20
Wim LeersFinally: this totally also belongs as a bugfix in 8.4.
Comment #21
larowlanIs 0 a valid key? If so, this should use ===
See https://3v4l.org/3V8ca
Comment #22
Wim LeersI … am not sure. But
\Drupal\Core\Config\Config::get()
usesif (empty($key))
, so let's just use that instead: then the mock behavior matches the implementation behavior.Comment #23
m4oliveiNice catch, empty() check sounds good. Here's an updated patch.
Comment #24
Wim LeersComment #25
larowlanSo 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
Comment #27
larowlanCommitted as bcb5afd and pushed to 8.5.x
Comment #28
Wim LeersQuoting @m4olivei from #6:
This is a problem on existing Drupal sites. Don't we want to commit this to 8.4 as well?
Comment #30
cburschkaTesting again for 8.4.
Comment #31
Wim LeersComment #33
larowlan8.4.x is critical fixes only now, this is already in 8.5.x - marking closed
Comment #35
Wim LeersDrupal 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! ❤️