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

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

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

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.

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

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?

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.

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

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

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.

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.

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

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

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

Hm.

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

Re-roll of #18

Kim

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?

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'?

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

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

:)

#24: file-public-path-settings-1856766-24.patch queued for re-testing.

#24: file-public-path-settings-1856766-24.patch queued for re-testing.

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

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

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

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

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

What just happened?

OK, that's a stupid mistake.

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?

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

Missing docs :)

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

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.

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.

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!

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

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.

Hmmm, I guess you're right.

Anyway, the code looks great. Thanks!

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.

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

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

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

+++ 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

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

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

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

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

Re-roll that addresses comments in #68.

I will fix this.

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

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

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.

Re-titled/updated issue summary.

I think we should close this in favour of #1887750: Use path relative to DRUPAL_ROOT in configuration directories

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