file_private_path should be in $settings, like file_public_path

Beta blocker too then I think.

When we get there, the change record should just get added to:
https://drupal.org/upgrade/file_public_path

Working on it.

Removed private.path config and replaced with the file_private_path settings.

Some things where you should pay *special* attention in your reviews:

1. In PrivateStream::basePath() I copied the pattern from PublicStream::basePath(), but not sure if in this case we need to set there the private folder for simpletests.
2. In the settings form, the default empty value looked inconsistent with the public one, so I added a "Not set" markup that maybe we should remove.
3. I added a system_upgrade_8061, and didn't remove any reference to the private config value in the other upgrade hooks. Because these should go into migrate, probably we need to do something different.
4. default.settings.php: please check grammar and clarity. I also added a link to the handbook for configuring secure private files, not sure if it is desired.

Forgot the most important replacement...

we might not need the hook_upgrade function here.
see #2168011: Remove all 7.x to 8.x update hooks and disallow updates from the previous major version

maybe we should do something else, like
a)
do a migration?
b)
Or add an entry to this g.d.o page: https://groups.drupal.org/node/394338 Drupal 7 updates that need Drupal 8 migrations ?

Actually the new migrate functionality requires a working site to migrate to. This should have already been done when you create the site to migrate to.

Perhaps we need a pre migrate step that if you're migrating from a drupal site checks things like this?

+++ b/core/modules/system/system.install
@@ -2284,6 +2285,15 @@ function system_update_8060() {
+function system_upgrade_8061() {
...
 /**
+ * Move the file_public_path variable to settings.
+ */
...
+  if ($path = update_variable_get('file_private_path')) {
+    drupal_rewrite_settings(array('settings' => array('file_private_path' => $path)));
+  }
+}

Hook_upgrade does not exist :) but I don't think we should endeavour to do this in migrate either.

The funny thing is that I copypasted that from system_upgrade_8060.
I'll ask @chx about the migration later, trying to fix the other errors first.

Removed "upgrade", Removed unnecessary test env checks in the PrivateStream.
Should still have lots of failures.

I cannot find the problem with the patch after debugging for long, so I'm running away for now. Please feel free to tackle.

/me is frustrated.

Working on this.

Above patch was not applying, attaching re-rolled patch.

The issue was that simpletest child site was never have the setting set so the private stream wrapper did not exist. We could also write the public path to settings.php and therefore no have the hack in PublicStream::basePath() ... but this exact change is covered in #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead

+++ b/core/modules/system/system.module
@@ -1914,7 +1914,7 @@
-  if (settings()->get('file_private_path')) {
+  if (\Drupal::config('system.file')->get('path.private')) {

This change in the interdiff look weird to me, but the complete patch looks ok.
RTBC++, but waiting for other reviews.

1. +++ b/core/lib/Drupal/Core/StreamWrapper/PrivateStream.php
   @@ -16,20 +16,29 @@
   +   *   The base path for private://, defaults to ''.
   +   */
   +  public static function basePath() {
   +    $base_path = settings()->get('file_private_path', '');
   +    return $base_path;
   +  }
   +
   

   I think we use Settings::getSingleton() in classes.

2. +++ b/core/lib/Drupal/Core/StreamWrapper/PrivateStream.php
   @@ -16,20 +16,29 @@
   +   *
   +   * @return string
   +   *   The base path for private://, defaults to ''.
   +   */
   +  public static function basePath() {
   +    $base_path = settings()->get('file_private_path', '');
   +    return $base_path;
   

   What does '' mean then? Unlike public path, using the private path is optional, it can be "not configured", shouldn't we use NULL for that?

   While I see the advantages, that's my main problem with the issue, almost nobody ever has to change the public path, but the private path needs to be configured before it can be used. This is making it quite a bit harder...

   Not saying that we shouldn't do it, just wanted to point it out, so that we're aware of it.

3. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateSystemConfigsTest.php
   @@ -184,7 +184,6 @@ public function testSystemFile() {
        $config = \Drupal::config('system.file');
   -    $this->assertIdentical($config->get('path.private'), 'files/test');
        $this->assertIdentical($config->get('path.temporary'), 'files/temp');
        \Drupal::configFactory()->setOverrideState($old_state);
   

   Looking at this, temp is actually at least as good a candidate for settings as private :)

4. +++ b/core/modules/system/lib/Drupal/system/Form/FileSystemForm.php
   @@ -36,12 +37,11 @@ public function buildForm(array $form, array &$form_state) {
        );
   ...
        $form['file_private_path'] = array(
   -      '#type' => 'textfield',
   +      '#type' => 'item',
          '#title' => t('Private file system path'),
   -      '#default_value' => $config->get('path.private'),
   -      '#maxlength' => 255,
   +      '#default_value' => PrivateStream::basePath(),
   +      '#markup' => (PrivateStream::basePath() ? PrivateStream::basePath() : t('Not set')),
          '#description' => t('An existing local file system path for storing private files. It should be writable by Drupal and not accessible over the web. See the online handbook for <a href="@handbook">more information about securing private files</a>.', array('@handbook' => 'http://drupal.org/documentation/modules/file')),
   -      '#after_build' => array('system_check_directory'),
   

   As a result of that, we should at least update the description here, as we did for public path already.

5. +++ b/sites/default/default.settings.php
   @@ -467,6 +467,19 @@
   + *
   + * A local file system path where private files will be stored. This directory
   + * must exist and be writable by Drupal. This directory must be absolute and
   + * outside of the the Drupal installation directory and not accessible over the
   + * web.
   + *
   + * See http://drupal.org/documentation/modules/file for more information about
   + * securing private files.
   + */
   +# $settings['file_private_path'] = '';
   +
   

   This doesn't seem very clear.

   *If* configured, then the directory must exist and be writable, but configuring it is optional, unlike the public directory.

20: 2170235.20.patch queued for re-testing.

Discussed this with @alexpott, we consider this now to be a blocker for #2353357: hook_stream_wrappers_alter() should be removed as it is broken since modules are not loaded on demand, as it allows us to remove the alter hook there completely and rely instead on a CoreServiceProvider to dynamically registering the service.

Here is a reroll for now and addressing my own review from a long time ago, will work on that next.

After this, we can then just remove the hook and are done in the other issue.

This is also performance relevant, as it is a config load that we currently have to do very early (if it would actually work, which is what the other issue is trying to fix).

Ok, changed to add this dynamically in CoreServiceProvider. Manual testing shows that this is working fine.

Unless I'm missing something the value should be removed from config schema as well. This will not fail validation because we do not complain on incomplete config, but we should remove it nonetheless.

@tstoeckler: Yeah, right, removed.

Also forgot to fix the settings name, this should fix most/all of those test fails.

Re-added the file_directory_path to the source, we need that so that the files tests are passing.

After understanding how this worked in migrate/D6, the private path migration was wrong anyway, as in D6, you had a single setting to say if your files are private or not, now we have two different directories, so updating the private path to the configured one unconditionally was wrong.

Instead, the correct approach is to migrate them into whatever is now configured for private/public on the target site.

Minor nitpicks - could be fixed on commit

1. +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
   @@ -21,7 +21,9 @@
   +use Symfony\Component\DependencyInjection\Definition;
   

   unused?

2. +++ b/core/modules/system/system.module
   @@ -14,6 +14,7 @@
   +use Drupal\Core\Site\Settings;
   

   Unused?

3. +++ b/sites/default/default.settings.php
   @@ -431,6 +431,18 @@
   + * A local file system path where private files will be stored. This directory
   + * must be absolute and outside of the the Drupal installation directory and
   

   nitpick: two ands here - change to 'A local file system path where private files will be stored. This directory must be absolute, outside of the the Drupal installation directory and not accessible over the web.' ?

Manual testing now

Manually tested - all clear

If bot is happy - +1 rtbc from me - minor nits above can be fixed on commit or re-roll

Bot is green so RTBC as per #39

+1 - committers, would you be able to fix nits in #38 on commit?

Fixed those nitpicks.

This will also need a change record, will write that tomorrow of nobody beats me to it. Shouldn't be hard, check the public file path change record that should already exist for inspiriation, explain that this is now in settings.php and maybe add that it needs to be set manually *before* private files are migrated from an old site. Also explain how to get the new path (PrivateStream::basePath()), change record applies to site builders and developers.

Draft change notice: https://www.drupal.org/node/2392959 ([#2392959])
Added beta info.

This brings into question what we should do with system.site:path.temporary. This is often set automatically by file_directory_temp(). Not quite sure what we should do with that.

    if (\Drupal::config('system.file')->get('path.private')) {
      $htaccess_files['private://.htaccess'] = array(
        'title' => t('Private files directory'),
        'directory' => drupal_realpath('private://'),
      );
    }

in system_requirements()...

Right...

I also updated the change record a bit, merged the upgrade and merge parts together, there's no upgrading from D7 :) https://www.drupal.org/node/2392959/revisions/view/7931789/7932211

Although I worked on this issue - that was months ago so I think it is okay if I commit it since this unblocks other criticals. This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed f41585c and pushed to 8.0.x. Thanks!