config_get_config_directory() does not use file public path setting but hard-codes it instead

Files: 
CommentFileSizeAuthor
#76 1856766_76.patch8.02 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 58,744 pass(es), 2 fail(s), and 0 exception(s). View
#76 interdiff.txt753 bytesmtift
#74 1856766_74.patch7.29 KBchx
FAILED: [[SimpleTest]]: [MySQL] 58,390 pass(es), 2 fail(s), and 0 exception(s). View
#74 interdiff.txt3.35 KBchx
#71 1856766_71.patch5.03 KBmtift
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#67 1856766_67.patch5.03 KBchx
FAILED: [[SimpleTest]]: [MySQL] 51,666 pass(es), 159 fail(s), and 65 exception(s). View
#64 interdiff.txt1.22 KBchx
#64 1856766_64.patch2.97 KBchx
FAILED: [[SimpleTest]]: [MySQL] 11,433 pass(es), 635 fail(s), and 22 exception(s). View
#62 1856766_62.patch2.29 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,252 pass(es). View
#60 1856766_60.patch2.29 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,365 pass(es). View
#58 1856766_58.patch1.14 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,006 pass(es). View
#54 1856766_54.patch24.77 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,304 pass(es). View
#54 1856766_54-interdiff.txt2.24 KBBerdir
#50 interdiff.txt1.95 KBchx
#50 1856766_50.patch24.77 KBchx
PASSED: [[SimpleTest]]: [MySQL] 57,837 pass(es). View
#47 1856766_47.patch24.12 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,123 pass(es). View
#47 interdiff.txt677 byteschx
#44 interdiff.txt15.51 KBchx
#44 1856766_44.patch23.97 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,063 pass(es). View
#43 1856766_43.patch21.02 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,105 pass(es). View
#43 interdiff.txt1.27 KBchx
#41 1856766_41.patch20.38 KBchx
FAILED: [[SimpleTest]]: [MySQL] 58,137 pass(es), 12 fail(s), and 0 exception(s). View
#39 1856766_38.patch20.36 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1856766_38.patch. Unable to apply patch. See the log in the details link for more information. View
#31 1856766_31.patch19.86 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1856766_31.patch. Unable to apply patch. See the log in the details link for more information. View
#24 file-public-path-settings-1856766-24.patch19.86 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file-public-path-settings-1856766-24.patch. Unable to apply patch. See the log in the details link for more information. View
#24 file-public-path-settings-1856766-24-interdiff.txt826 bytesBerdir
#22 file-public-path-settings-1856766-22.patch19.36 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,831 pass(es), 131 fail(s), and 75 exception(s). View
#22 file-public-path-settings-1856766-22-interdiff.txt516 bytesBerdir
#19 1856766-file-public-path-19.patch19.82 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 53,205 pass(es). View
#18 1856766-file-public-path-18.patch19.82 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 52,219 pass(es). View
#18 1856766-file-public-path-18-interdiff.txt1.5 KBBerdir
#15 1856766-file-public-path-15.patch19.62 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 52,060 pass(es), 99 fail(s), and 17 exception(s). View
#15 1856766-file-public-path-15-interdiff.txt1.57 KBBerdir
#11 1856766-file-public-path-11.patch19.5 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 51,404 pass(es), 3 fail(s), and 0 exception(s). View
#5 file-public-path-setting-1856766-5.patch17.97 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,156 pass(es). View
#5 file-public-path-setting-1856766-5-interdiff.txt1.03 KBBerdir
#3 file-public-path-setting-1856766-3.patch18.29 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,655 pass(es), 16 fail(s), and 18 exception(s). View

Comments

typhonius’s picture

Status: Active » Closed (duplicate)
heyrocker’s picture

Status: Closed (duplicate) » Active
Issue tags: +Configuration system

Reopening this since the public file path was removed from #1496480: Convert file system settings to the new configuration system

Berdir’s picture

Status: Active » Needs review
FileSize
18.29 KB
FAILED: [[SimpleTest]]: [MySQL] 49,655 pass(es), 16 fail(s), and 18 exception(s). View

Here's a start to convert this. Added a helper function to avoid duplicating the default value in a million places and support the same within-test override as we have for config_get_config_directory().

Status: Needs review » Needs work

The last submitted patch, file-public-path-setting-1856766-3.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
17.97 KB
PASSED: [[SimpleTest]]: [MySQL] 50,156 pass(es). View

Ok, the PhpStorageFactory.php obviously also need to check for an in-test override. So we either need to make sure that we always pass in a directory (at least when we're in a test) or also use file_public_path(). I've done the second for now, to make sure that this does fix the tests, which I think it should.

Berdir’s picture

I think two things should be answered to get this in:

- Should we add that file_public_path() method to handle the default and in-test override? Alternatively, we could also write the default/test-value into $settings.
- What do we want to do with PhpStorageFactory? Ensuring the settings()->get('file_public_path') always returns the right path would allow to simply inject that, or we could make sure that the directory is always set outside of it or keep it like that...

Dave Reid’s picture

I like the addition of the helper function, but considering it contains testing-only code just really sticks out to me as wrong. Is it absolutely necessary?

Berdir’s picture

It is the same logic as config_get_config_directory() basically, in both cases, we need *something* as it depends on something that can only be changed in settings.php otherwise and we currently can not write a custom one for tests.

The only alternative is overwriting the setting from the outside during bootstrap, similar to how we overwrite the database settings.

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community

I actually think this is fine as it is, if we want to tweak it we can do it in followups. Thanks!

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/bootstrap.inc
@@ -3493,6 +3493,19 @@ function drupal_check_memory_limit($required, $memory_limit = NULL) {
+function file_public_path() {
+  if ($test_prefix = drupal_valid_test_ua()) {
+    // @see Drupal\simpletest\WebTestBase::setUp()
+    return conf_path() . '/files/simpletest/' . substr($test_prefix, 10);
+  }
+  else {
+    return settings()->get('file_public_path', conf_path() . '/files');
+  }
+}

Hm. I understand the intention, but this looks extremely dynamic to me.

E.g., the test setup + teardown procedures need to be really really careful and call file_public_path() in the right/correct moments. If it's called too early, problem. If it's called too late, problem.

At the very least, we'd have to add some extra validation to the test environment setup/teardown functions to make sure that the returned path is the expected path (i.e., when the simpletest path is expected, it must contain the literal string "/simpletest/").

Additionally, the first returned path appears to be wrong: The public files path of a test environment is within the public files path of the parent-site/test-runner. The file_public_path from settings is the base path in both cases.

+++ b/core/modules/system/system.admin.inc
@@ -1752,11 +1752,11 @@ function system_clear_page_cache_submit($form, &$form_state) {
   $form['file_public_path'] = array(
-    '#type' => 'textfield',
+    '#type' => 'item',
     '#title' => t('Public file system path'),
-    '#default_value' => variable_get('file_public_path', conf_path() . '/files'),
+    '#markup' => file_public_path(),
     '#maxlength' => 255,
-    '#description' => t('A local file system path where public files will be stored. This directory must exist and be writable by Drupal. This directory must be relative to the Drupal installation directory and be accessible over the web.'),
+    '#description' => t('A local file system path where public files will be stored. This directory must exist and be writable by Drupal. This directory must be relative to the Drupal installation directory and be accessible over the web. This must be changed in settings.php'),
     '#after_build' => array('system_check_directory'),

1) I do not see a change to settings.php in this patch? If this setting is expected to be contained and configured there, then we should have a proper section for it by default in settings.php.

2) Likewise, the installer does not write this value?

3) Upgrade path?

4) The #after_build seems to be obsolete, since the setting cannot be changed. EDIT: #maxlength, too.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
19.5 KB
FAILED: [[SimpleTest]]: [MySQL] 51,404 pass(es), 3 fail(s), and 0 exception(s). View

This patch does the following:

- Modifies base path of simpletest public file path
- Adds the setting to default.settings.php with a default value
- Removes #after_build and #maxlength
- Fixes testFileConfigurationPage() that had been recently added but was broken with this patch. I think this is the right way to do that since its just a read-only value now, but verification would be nice.

At the very least, we'd have to add some extra validation to the test environment setup/teardown functions to make sure that the returned path is the expected path (i.e., when the simpletest path is expected, it must contain the literal string "/simpletest/").

I'm not really sure this is necessary, or I don't understand why this different than what we're doing in config_get_config_directory().

2) Likewise, the installer does not write this value?

Since we provide a default in default.settings.php in this patch, the installer shouldn't have to write anything.

3) Upgrade path?

None of the other $settings conversions has provided an upgrade path either, and it looks a bit hairy. Ideally we could change drupal_rewrite_settings() to allow passing in $settings variables but that seems like a bigger issue than we want to tackle here. I'd rather push that to a followup and do the upgrade path for all the $settings at once there. I've created #1921818: Modify drupal_rewrite_settings() to allow writing $settings values and #1921822: Take account of drupal_hash_salt during migration path from 7.x for that.

chx’s picture

I have more than once hit the wall with trying to write $conf['foo'] = 'bar' from install with drupal_rewrite_settings. This is not easy so moving it to a followup or followups is absolutely sensible.

Status: Needs review » Needs work

The last submitted patch, 1856766-file-public-path-11.patch, failed testing.

heyrocker’s picture

I started looking into those fails but I have to admit I'm dumbfounded. Any one else?

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
19.62 KB
FAILED: [[SimpleTest]]: [MySQL] 52,060 pass(es), 99 fail(s), and 17 exception(s). View
+++ b/core/includes/bootstrap.incundefined
@@ -3494,6 +3494,20 @@ function drupal_check_memory_limit($required, $memory_limit = NULL) {
+  $base_path = settings()->get('file_public_path', conf_path() . '/files');
+  if ($test_prefix = drupal_valid_test_ua()) {
+    // @see Drupal\simpletest\WebTestBase::setUp()
+    return $base_path . '/simpletest/' . substr($test_prefix, 10);

The problem with this is that this now returned nested directories within the test (as we explicitly set the file public path there) but not on page callbacks.

However, doing this means that we don't need to set the file_public_path at all in setUp(), so removed that. And we also need to directly use settings()->get() when detecting the original directory.

Note that this change means that simpletest directories for nested tests are now no longer nested but are on the same level with a different test id, just like parallel tests are too.

Status: Needs review » Needs work

The last submitted patch, 1856766-file-public-path-15.patch, failed testing.

Berdir’s picture

Hm, looks like this breaks other tests, so removing the setting doesn't fix it properly either.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
19.82 KB
PASSED: [[SimpleTest]]: [MySQL] 52,219 pass(es). View

Hm.

I think I got it this time. It's a bit ugly, but I'm not sure if anything else will work.

kim.pepper’s picture

FileSize
19.82 KB
PASSED: [[SimpleTest]]: [MySQL] 53,205 pass(es). View

Re-roll of #18

Kim

chx’s picture

Status: Needs review » Needs work

I am reasonably sure that $settings['file_public_path'] = conf_path() . '/files'; in settings.php will a) eventually have some really unfun side effect -- the thing is, if you have a multisite, it might not be a terribly good idea to have it dynamic... b) totally unnecessary anyways cos settings()->get() default just contains it. What about adding # $settings['file_public_path'] = 'sites/default/files'; instead?

Berdir’s picture

Status: Needs work » Needs review
FileSize
516 bytes
19.36 KB
FAILED: [[SimpleTest]]: [MySQL] 53,831 pass(es), 131 fail(s), and 75 exception(s). View

Re-roll with settings definition in default.settings.php changed as suggested.

Wondering if we can use the new test settings.php/confg path for this now. It might just work if we move the public files path to $base_path . '/simpletest/' . substr($test_prefix, 10) . '/files'?

Status: Needs review » Needs work

The last submitted patch, file-public-path-settings-1856766-22.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
826 bytes
19.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file-public-path-settings-1856766-24.patch. Unable to apply patch. See the log in the details link for more information. View

Messed up the settings set in setUp() during the re-roll.

effulgentsia’s picture

Title: Convert file_public_path to the new configuration system » Convert file_public_path to the new settings system (make it not configurable via UI)

I support the approach in #24, but retitling issue to reflect it.

effulgentsia’s picture

Title: Convert file_public_path to the new settings system (make it not configurable via UI) » Convert file_public_path to the new settings system (make it not configurable via UI or YAML)

:)

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, file-public-path-settings-1856766-24.patch, failed testing.

moshe weitzman’s picture

Status: Needs work » Needs review

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

The last submitted patch, file-public-path-settings-1856766-24.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
19.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1856766_31.patch. Unable to apply patch. See the log in the details link for more information. View

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

The last submitted patch, 1856766_31.patch, failed testing.

moshe weitzman’s picture

Status: Needs work » Needs review

#31: 1856766_31.patch queued for re-testing.

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

The last submitted patch, 1856766_31.patch, failed testing.

dburiak’s picture

We are working now with this issue during Code Sprint UA.

chx’s picture

Great! I am not even sure whether that was a valid test fail; please find me in IRC if you need any help.

jibran’s picture

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

#31: 1856766_31.patch queued for re-testing.

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

The last submitted patch, 1856766_31.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
20.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1856766_38.patch. Unable to apply patch. See the log in the details link for more information. View

I am not sure how the hell this can be "normal" when #1859110: Circular dependencies and hidden bugs with configuration overrides probably depends on this...

Status: Needs review » Needs work

The last submitted patch, 1856766_38.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
20.38 KB
FAILED: [[SimpleTest]]: [MySQL] 58,137 pass(es), 12 fail(s), and 0 exception(s). View

What just happened?

Status: Needs review » Needs work

The last submitted patch, 1856766_41.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
21.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,105 pass(es). View

OK, that's a stupid mistake.

chx’s picture

FileSize
23.97 KB
PASSED: [[SimpleTest]]: [MySQL] 58,063 pass(es). View
15.51 KB

And here's a version for the OOP zealots. Obviously, the patch and the resulting code is some 15% percent larger but we can't have a function call in a class, right? Everything, including code bloat and inscrutable code is better, isn't it?

Berdir’s picture

+++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
@@ -16,20 +16,29 @@
+
+  static public function basePath() {

Missing docs :)

chx’s picture

FileSize
677 bytes
24.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,123 pass(es). View

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

The last submitted patch, 1856766_47.patch, failed testing.

jibran’s picture

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

#47: 1856766_47.patch queued for re-testing.

chx’s picture

FileSize
24.77 KB
PASSED: [[SimpleTest]]: [MySQL] 57,837 pass(es). View
1.95 KB

I have written an update function. It's pointless to test it. Also, run-tests.sh test coverage is um. Not stellar. Not sure what can be done, of course.

Berdir’s picture

Priority: Normal » Critical

Ok, this is fixing a pretty serious crazyness that is currently blocking #1862202: Objectify the language system, see comment #142. Most notably, the public file path is not always within the simpletest files directory, so we're looking in different places depending on when it's called during bootstrap, that results in too many container dumps and flushes/deleteAll() calls that don't work.

Raising to critical, need to find someone that can RTBC this to unblock the other issue.

tim.plunkett’s picture

  1. +++ b/core/includes/file.inc
    @@ -385,7 +386,7 @@ function file_stream_wrapper_get_instance_by_uri($uri) {
    + * @return Drupal\Core\StreamWrapper\StreamWrapperInterface
    

    \Drupal

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
    @@ -16,20 +16,35 @@
    +   * Returns the base path for public://
    

    Missing trailing .

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
    @@ -16,20 +16,35 @@
    +   *   the base path for public:// typically sites/default/files.
    

    Capital T in The

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
    @@ -16,20 +16,35 @@
    +  static public function basePath() {
    

    Most of our statics are public static function

  5. +++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
    @@ -16,20 +16,35 @@
    +    return $base_path;
    +  }
    

    Missing blank line at the end of the file

  6. +++ b/core/modules/system/lib/Drupal/system/Form/FileSystemForm.php
    @@ -27,12 +28,11 @@ public function getFormID() {
    -      '#type' => 'textfield',
    +      '#type' => 'item',
    

    Womp womp. This is going to suck for site builders...

  7. +++ b/core/modules/system/lib/Drupal/system/Form/ThemeSettingsForm.php
    @@ -240,7 +241,7 @@ public function buildForm(array $form, array &$form_state, $theme_name = '') {
    -          $local_file = strtr($original_path, array('public:/' => variable_get('file_public_path', conf_path() . '/files')));
    +          $local_file = strtr($original_path, array('public:/' => PublicStream::basePath()));
    

    This is in the original code, but why only one slash?

  8. +++ b/core/modules/system/system.install
    @@ -2242,6 +2243,15 @@ function system_update_8059() {
    + * Move the file_public_path variable to settings.
    + */
    +function system_upgrade_8060() {
    +  if ($path = update_variable_get('file_public_path')) {
    +    drupal_rewrite_settings(array('settings' => array('file_public_path' => $path)));
    +  }
    +}
    

    Nice!

beejeebus’s picture

overall, this looks great. one tiny nitpick:

+      $this->testConnection->copyDirectory($source, DRUPAL_ROOT . '/'. PublicStream::basePath());

missing space.

Berdir’s picture

FileSize
2.24 KB
24.77 KB
PASSED: [[SimpleTest]]: [MySQL] 58,304 pass(es). View

Fixed nitpicks.

- I don't think it's a big problem for site builders, how many are actually changing this? And it's actually getting quite complicated to do it in 8.x, the config directory is within the *default* public site directory, so you can't move that. I have a separate issue to separate that from this completely and use a relative path to DRUPAL_ROOT instead but that didn't make it :(

- The single slash in public:/ is because we want to replace public://whatever with '/some/path/bla' . '/' . 'whatever', so the path we're replacing it with doesn't have a trailing slash and so we keep the second of public:// instead of replacing it with a hardcoded /.

catch’s picture

This isn't remotely a problem for sitebuilders, the fact we allow this to be set in the UI at all is the problem. When you change this setting, no files get moved, no hard-coded links in textareas get updated etc. etc. so you can easily end up with a broken site. The only case where you might be OK is if you don't have any public files at all yet.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Hmmm, I guess you're right.

Anyway, the code looks great. Thanks!

alexpott’s picture

Title: Convert file_public_path to the new settings system (make it not configurable via UI or YAML) » Change notice: Convert file_public_path to the new settings system (make it not configurable via UI or YAML)
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 5460c03 and pushed to 8.x. Thanks!

We need a change notice detailing this change because whilst a site builder should never alter this setting they could in the past.

chx’s picture

Title: Change notice: Convert file_public_path to the new settings system (make it not configurable via UI or YAML) » Convert file_public_path to the new settings system (make it not configurable via UI or YAML)
FileSize
1.14 KB
PASSED: [[SimpleTest]]: [MySQL] 58,006 pass(es). View

Erm, nope. We call drupal_install_config_directories(); before drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE);

chx’s picture

Status: Active » Needs review
Issue tags: -Configuration system, -Needs change record +D8 upgrade path
chx’s picture

FileSize
2.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,365 pass(es). View

Actually, I need to renumber the rest of UPGRADE.txt

Berdir’s picture

config_get_config_directory() and therefore drupal_install_config_directories() does not rely on the file_public_path at the moment. It hardcodes "conf_path() . '/files/'". However, that won't actually work and will explode if you don't have a files directory there.

But we either need to change that and use the setting there or we can keep he upgrade path, knowing that it will be broken ;)

chx’s picture

FileSize
2.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,252 pass(es). View
tstoeckler’s picture

+++ b/core/UPGRADE.txt
@@ -220,17 +223,17 @@ following the instructions in the INTRODUCTION section at the top of this file:
+2ö. Go to Administration > Configuration > Development > Maintenance mode.

Typo: 2ö -> 20

chx’s picture

FileSize
2.97 KB
FAILED: [[SimpleTest]]: [MySQL] 11,433 pass(es), 635 fail(s), and 22 exception(s). View
1.22 KB

Changed config_get_config_directory to use settings()->get('file_public_path', conf_path() . '/files');

Status: Needs review » Needs work

The last submitted patch, 1856766_64.patch, failed testing.

Tor Arne Thune’s picture

Status: Needs work » Needs review
+++ b/core/UPGRADE.txtundefined
@@ -203,7 +203,10 @@ following the instructions in the INTRODUCTION section at the top of this file:
+   https//drupal.org/upgrade/file_public_path

Missing : after https.

chx’s picture

FileSize
5.03 KB
FAILED: [[SimpleTest]]: [MySQL] 51,666 pass(es), 159 fail(s), and 65 exception(s). View
Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/UPGRADE.txt
    @@ -203,7 +203,10 @@ following the instructions in the INTRODUCTION section at the top of this file:
    +   https//drupal.org/upgrade/file_public_path
    

    Still missing a ":" in here :(

  2. +++ b/core/includes/bootstrap.inc
    @@ -431,7 +431,7 @@ function config_get_config_directory($type = CONFIG_ACTIVE_DIRECTORY) {
    -      $path = conf_path() . '/files/' . $config_directories[$type]['path'];
    +      $path = settings()->get('file_public_path', conf_path() . '/files') . '/' . $config_directories[$type]['path'];
    

    Is it not possible to use the static method here?

    Should at least use Drupal::settings().

webchick’s picture

Issue tags: +beta blocker

Tagging all critical D8 upgrade path issues as "beta blocker."

catch’s picture

Title: Convert file_public_path to the new settings system (make it not configurable via UI or YAML) » config_get_config_directory() does not use file public path variable
Category: task » bug
mtift’s picture

Status: Needs work » Needs review
FileSize
5.03 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Re-roll that addresses comments in #68.

Status: Needs review » Needs work

The last submitted patch, 1856766_71.patch, failed testing.

chx’s picture

Assigned: Unassigned » chx

I will fix this.

chx’s picture

Status: Needs work » Needs review
FileSize
3.35 KB
7.29 KB
FAILED: [[SimpleTest]]: [MySQL] 58,390 pass(es), 2 fail(s), and 0 exception(s). View

This patch is dedicated to the wonderful music sToa: Candide without which I could never figure this out.

Status: Needs review » Needs work

The last submitted patch, 1856766_74.patch, failed testing.

mtift’s picture

Status: Needs work » Needs review
FileSize
753 bytes
8.02 KB
FAILED: [[SimpleTest]]: [MySQL] 58,744 pass(es), 2 fail(s), and 0 exception(s). View

So maybe something like this will get us closer. I have some serious doubts.

Status: Needs review » Needs work

The last submitted patch, 1856766_76.patch, failed testing.

ianthomas_uk’s picture

I spotted this in \Drupal\system\Tests\InstallerTest::setUp()

    // Reload config directories.
    include $this->public_files_directory . '/settings.php';
    $prefix = substr($this->public_files_directory, strlen(conf_path() . '/files/'));
    foreach ($config_directories as $type => $data) {
      $GLOBALS['config_directories'][$type]['path'] = $prefix . '/files/' . $data['path'];
    }

$config_directories is not defined though. I don't know enough about this code to know how/where it should be defined (my guess would be that we just call global $config_directories, as there is a similar change to another test in this patch.

chx’s picture

Assigned: chx » Unassigned
catch’s picture

Title: config_get_config_directory() does not use file public path variable » config_get_config_directory() does not use file public path setting
Issue summary: View changes

Re-titled/updated issue summary.

alexpott’s picture

Status: Needs work » Closed (won't fix)
Berdir’s picture

Title: config_get_config_directory() does not use file public path setting » Convert file_public_path to the new settings system (make it not configurable via UI or YAML)
Status: Closed (won't fix) » Fixed

Restoring the original issue title, this is one of the critical-follow-ups that you like so much ;)

Status: Fixed » Closed (fixed)

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

sun’s picture