Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
This is because the config_get_config_directory() function always prepends conf_path() to the path.
function config_get_config_directory($type = CONFIG_ACTIVE_DIRECTORY) {
global $config_directories;
if ($test_prefix = drupal_valid_test_ua()) {
// @see Drupal\simpletest\WebTestBase::setUp()
$path = conf_path() . '/files/simpletest/' . substr($test_prefix, 10) . '/config_' . $type;
}
elseif (!empty($config_directories[$type])) {
$path = conf_path() . '/files/' . $config_directories[$type];
}
else {
throw new Exception(format_string('The configuration directory type %type does not exist.', array('%type' => $type)));
}
return $path;
}
The current comments in default.settings.php make a claim that is currently impossible...
* By default, Drupal configuration files are stored in a randomly named
* directory under the default public files path. On install the
* named directory is created in the default files directory. For enhanced
* security, you may set this variable to a location outside your docroot.
Comment | File | Size | Author |
---|---|---|---|
#35 | qadrupalorg.jpg | 68.92 KB | alexpott |
#28 | 1772708-28.drupal8.config-path.patch | 3.83 KB | alexpott |
#28 | diff-22-28.txt | 697 bytes | alexpott |
#22 | 20-22-interdiff.txt | 1.92 KB | alexpott |
#22 | 1772708-22.drupal8.config-path.patch | 3.84 KB | alexpott |
Comments
Comment #1
sunComment #2
alexpottA little patch :)
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commenteddon't we want is_dir() instead of file_exists()?
Comment #4
sunNot sure... is_dir() would exclude symlinked directories - do we want to prevent that?
Comment #5
sunHm. http://php.net/manual/en/function.is-dir.php seems to disagree with me -- looks my info on file_exists() vs. is_dir() is outdated?
Comment #6
gddI think this is a major (although it looks like it will be solved here pretty quick)
Comment #7
alexpottHere is a patch with is_dir... I used file_exists in the initial patch because that is what Drupal uses all over the place to check if a directory exists. Reading http://php.net/manual/en/function.is-dir.php it is blatant that we should be using is_dir :)
Comment #8
sunThis will cause two filestats on every bootstrap. One for the active directory and one for the staging directory.
However, config_get_config_directory() is not called from anywhere else during regular runtime:
http://api.drupal.org/api/drupal/core!includes!bootstrap.inc/function/ca...
I tried to think through alternatives, e.g., determining whether the path is absolute by other file path semantics, but doing that in a cross-platform compatible way quickly becomes very complex.
So I think this is good to go.
Comment #9
gddIf catch is worried about the performance potential, we could just do 'if the directory begins with DIRECTORY_SEPARATOR then don't add conf_path() to it.' This is less robust, but if we document it in default.settings.php it should be good enough.
Comment #10
sun...with the only exception that absolute file paths on Windows do not start with DIRECTORY_SEPARATOR, but rather
DRIVE_LETTER:DIRECTORY_SEPARATOR
...Comment #11
gddThey actually do sometimes start with DIRECTORY_SEPARATOR (\\server\folder\in\here\somewhere) but I think we can code in the drive letter situation as well
Comment #12
catchAt least for file_exists(), if the file doesn't exist, then it doesn't get cached in the realpath cache so it'll be an actual filesystem hit each request (and no idea how the filesystem handles those, but I'd rather not rely on that).
I'd assume the same is true for is_dir() - it ought to be possible to check this by using realpath_cache_get() and a test script.
In terms of what Drupal does it's not very much, but if we can avoid it that'd be great. heyrocker's suggestion seems fine to me.
Comment #13
alexpottHow about this? Just add the information to
$config_directories
Comment #14
chx CreditAttribution: chx commentedI would do
and then you do not need to write it out explicitly.
Comment #15
chx CreditAttribution: chx commentedActually: disregard.
Comment #16
alexpottagree.
Comment #18
alexpottNeed to fix the config directory creation in simpletest.
Comment #20
alexpottThere was more :)
Comment #21
sun$this->configDirectories is actually supposed to contain the resolved paths already. Can we retain that?
Comment #22
alexpottimho it does... just with an extra key :)....
But I'm not fussed... so attached patch does what I think you want.
Comment #23
sunThanks! :)
Yeah, if the original definition is needed for any reason in a test, then
$GLOBALS['config_directories']
is always available. However, the primary use-case in tests is to work on the already resolved paths (which is also why$this->originalFileDirectory
is being prepended to$this->configDirectories
but not to the global variable).Comment #24
chx CreditAttribution: chx commentedOh very nice I was worried about this not being documented anywhere that's why I tried to withdraw my proposal but I am happy we went with it at the end AND it's documented as well.
Comment #25
webchickCatch had his paws on this issue earlier, so figure it best for him to finish it off, but I believe this addresses his concerns. Woot!
Comment #26
catch#22: 1772708-22.drupal8.config-path.patch queued for re-testing.
Comment #28
alexpottrerolled patch... interdiff not very helpful... as there is no change to the patch setting back to RTBC
attaching actually diff of patches... the change below is not due to this patch (but is the reason #22 no longer applies!)
Comment #29
catchThanks for the quick re-roll. Committed/pushed to 8.x.
Comment #30
catchThis may have broken the test bot, reverted, moving back to CNR so we can figure it out.
Comment #31
alexpottIt did break it :( Everything came back after the revert
Comment #32
sun#28: 1772708-28.drupal8.config-path.patch queued for re-testing.
Comment #33
sunWhat exactly got broken? Did anyone copy the review log of a failing test or captured a screenshot?
Comment #34
cosmicdreams CreditAttribution: cosmicdreams commented#28: 1772708-28.drupal8.config-path.patch queued for re-testing.
Comment #35
alexpottAbove is a screenshot what I remember...
There was no other information on the screen... no error... no log... no nothing... just the MySQL status showing neither fail or pass... think it said queueing.
Comment #36
sunI suspect that the 8.x branch test failed on something completely different; e.g., #1779638: Unexplained test failure in LocaleFileImportStatus->testBulkImportUpdateExisting() - which in turn broke HEAD.
Let's try to commit this again and ensure to get sufficient debugging info in case it breaks again.
Comment #37
jthorson CreditAttribution: jthorson commentedRe #28: I have three logs for this test, and all look like normal runs with zero fails and no error messages.
The description in #35 sounds like the test had probably already been requeued when alexpott looked at it ... we've been pretty free with granting access to the qa.d.o re-test ability, so not all re-queues will be reflected in the issue comments.
It is also possible that we've introduced some new issues with results getting back to qa.d.o with the rollout of #1757668: Multiple commits result in unreliable testing ... We don't yet have a staging environment with full git functionality, so this went in with virtually no testing. Unfortunately, our watchdog log has rolled over, so I can't see whether the D8 test might have been re-queued while this was testing (the suspicion being that a re-queue of D8 HEAD while this is testing might somehow interfere with the reporting of test results back to qa.d.o). Will monitor.
Comment #38
webchickOk, trying that again.
Committed and pushed to 8.x.