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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +Configuration system
alexpott’s picture

Status: Active » Needs review
FileSize
880 bytes

A little patch :)

Anonymous’s picture

Status: Needs review » Needs work

don't we want is_dir() instead of file_exists()?

sun’s picture

Status: Needs work » Needs review

Not sure... is_dir() would exclude symlinked directories - do we want to prevent that?

sun’s picture

Hm. 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?

gdd’s picture

Priority: Normal » Major

I think this is a major (although it looks like it will be solved here pretty quick)

alexpott’s picture

Here 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 :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

This 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.

gdd’s picture

If 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.

sun’s picture

...with the only exception that absolute file paths on Windows do not start with DIRECTORY_SEPARATOR, but rather DRIVE_LETTER:DIRECTORY_SEPARATOR...

gdd’s picture

They 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

catch’s picture

Status: Reviewed & tested by the community » Needs review

At 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.

alexpott’s picture

How about this? Just add the information to $config_directories

chx’s picture

I would do

+    if (empty($config_directories[$type]['absolute'])) {
+      $path = conf_path() . '/files/' . $config_directories[$type]['path'];
+    }
+    else {
+      $path = $config_directories[$type]['path'];

and then you do not need to write it out explicitly.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Actually: disregard.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.46 KB
1.22 KB

agree.

Status: Needs review » Needs work

The last submitted patch, 1772708-15.drupal8.config-path.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.26 KB
681 bytes

Need to fix the config directory creation in simpletest.

Status: Needs review » Needs work

The last submitted patch, 1772708-18.drupal8.config-path.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.63 KB
2.28 KB

There was more :)

sun’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -733,11 +733,11 @@
-      $this->configDirectories[$type] = $this->originalFileDirectory . '/' . $path;
+      $this->configDirectories[$type]['path'] = $this->originalFileDirectory . '/' . $directory['path'];

$this->configDirectories is actually supposed to contain the resolved paths already. Can we retain that?

alexpott’s picture

imho it does... just with an extra key :)....

But I'm not fussed... so attached patch does what I think you want.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! :)

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).

chx’s picture

Oh 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.

webchick’s picture

Assigned: Unassigned » catch

Catch 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!

catch’s picture

Issue tags: -Configuration system

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration system

The last submitted patch, 1772708-22.drupal8.config-path.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
697 bytes
3.83 KB

rerolled 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!)

> @@ -735,11 +735,11 @@ abstract class TestBase {
65c65
<      // Log fatal errors.
---
>      // Unset globals.
67c67
catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the quick re-roll. Committed/pushed to 8.x.

catch’s picture

Status: Fixed » Needs review

This may have broken the test bot, reverted, moving back to CNR so we can figure it out.

alexpott’s picture

It did break it :( Everything came back after the revert

sun’s picture

sun’s picture

What exactly got broken? Did anyone copy the review log of a failing test or captured a screenshot?

cosmicdreams’s picture

alexpott’s picture

FileSize
68.92 KB

qa drupal org screenshot

Above 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.

sun’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

jthorson’s picture

Re #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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, trying that again.

Committed and pushed to 8.x.

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