Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class

.

This would be a lot better if #2028109: Convert hook_stream_wrappers() to tagged services. was in. Here's a first pass.

I'll fix the @todo :)

Drupal::config() is our best bet.

Yes, the rest of that will be plugins once the other issue goes in.

Not all of the uri/stream functions are going to be straight plugins, they themselves will also need to be wrappers.
I'd like to just postpone this on #2028109: Convert hook_stream_wrappers() to tagged services. for right now.

Just tracking some work, this is what it will look like once the other issue is in.

I'm very concerned about \Utility turning into a dumping-ground of arbitrary things.

In my mind, \Utility should not contain any classes that depend on other (injected-or-not) services.

So e.g. the utility/helper classes for arrays, array-sorting, string escaping, etc are all fine, because they are self-contained (having only PHP core as a dependency).

But the procedural functions being converted here clearly do not follow that pattern — since they rely and depend on configuration, plugins, and other things, they are part of an own "service", which needs proper dependency injection.

Can we move this code from \Utility into \File, register it as a service and inject the dependencies?

In the end (and hopefully before release), we should ideally consolidate all the file related things into that namespace:

Drupal\Core\File\StreamWrapper\...
Drupal\Core\File\Transfer\...
Drupal\Core\File\FileHandler (or similar; the stuff here)

I don't think we should bother waiting around for #2028109: Convert hook_stream_wrappers() to tagged services.

Anyone working on this? file_unmanaged_copy() right now is unsuitable for use in migration (doesn't work for remote files - see #2244555: Use copy() directly instead of file_unmanaged_copy()), not sure if we should refactor it on its own or as part of this issue.

I'm working on this.

Here's a fresh start.

1. How was decided which functions to move and which not?
2. How far should this issue go in cleaning up e.g. code style, outdated comments and such?

ad 1)

• It looks like explicit stream wrapper functions are left in file.inc, but then I don't see a real difference between file_stream_wrapper_get_class() vs file_stream_wrapper_valid_scheme()?
• file_uri_scheme() vs file_uri_target()
• ...

ad 2)

• Some comment lines now exceed the 80 character limit due to the extra indentation.
• Many parameters and return values are not documented with type information as is now the standard.
• getDirectoryPath() in method tempnam() can fatal out if the directory does not belong to a LocalStream type stream wrapper.
• PHP's chmod does support stream wrappers as of PHP 5.4. This part of the documentation for the chmod() method is out dated (after #2107287: PHP 5.4 calls a new stream_metadata() method on stream wrappers not implemented by Drupal).
• drupal.org is referred to via http, not https.

1) I left the stream wrapper functions that actual need the info/instances of stream wrappers. The rest was moved.
2) I will probably go over this for violations caused by the patch, like 80 chars, but not fixing the rest of it here.

either this or #2043773: Replace php function wrappers in file.inc with a Drupal\Core\File\FileSystem class should be closed..those two issues are very similar

+++ b/core/includes/file.inc
index 0000000..32746e0
--- /dev/null

--- /dev/null
+++ b/core/lib/Drupal/Core/Utility/File.php

i really think Drupal/Core/File is a better namespace for this

Marked the other issue as a dupe.
Per #8, made this a service. Was able to add tests for chmod and unlink, thanks to vfsStream.

Forgot to test locally before posting. Missed a service.

FWIW: adding

  $container
    ->register('file_system', 'Drupal\Core\File\FileSystem')
    ->addArgument(Settings::getInstance())
    ->addArgument((new Drupal\Core\Logger\LoggerChannelFactory())->get('file'));

to function install_begin_request() does make the error go away.

So it is an order problem during installation: the file_system service is not yet there, but already used. Makes me think, can I define a logger that logs to private:\\mylogfile.log and would that give problems here or in #2028109: Convert hook_stream_wrappers() to tagged services.?

Services used during the installer need to be registered, so your code is correct.

We can't set up the config directories (using the file system) until the container is built.

Seems like TestBase is trying to clean up sites/simpletest/* after the container is reset?

Reroll.

This is much easier to debug with #2363341: Throw exception in Drupal::service() and friends, if container not initialized yet. applied.

Reroll. interdiff is ot a real interdiff, but a diff of the changes that failed to apply (and that I applied manually).

The logger (factory) has a lot of (new) dependencies, but with this change I could reinstall my local test env (via the UI). Let's see if the tests pass.

It is a hack, but just to see if all failures are a result of the same problem.

Problem: file_uri_scheme() get's called during a web based test setup. Stack trace (not complete):
- WebTestBase::setUp()
- WebTestBase::installParameters()
- drupal_get_database_types()
- file_scan_directory()
- file_stream_wrapper_uri_normalize()
- file_uri_scheme
- \Drupal::service('file_system')
- static::$container->get($id);

And this only to find out if there is more than 1 database driver. I would think that even if there's 1, it won't hurt to select it on the install form (and if it is not there, that web test form fill in handling takes care of fields that are not found on the form).

I give up. It looks like the remaining errors are Drupal test setup specific and I do not have enough knowledge about Drupal testing to solve these remaining errors in an easy way. Someone else who is willing to give it a try?

Remaining failures:
Failure in core\modules\system\src\Tests\Bootstrap\GetFilenameUnitTest.php:
Failure in core\modules\system\src\Tests\Routing\RouteProviderTest.php:
- TestBase::restoreEnvironment()
- file_unmanaged_delete_recursive()
- drupal_rmdir()

TestBase::prepareEnvironment() unsets the container:

    // Ensure there is no service container.
    $this->container = NULL;
    \Drupal::setContainer(NULL);

No idea why, but this probably provokes the errors.

Failure in core\modules\system\src\Tests\DrupalKernel\DrupalKernelTest.php:
- DrupalKernelTest::setUp()
- KernelTestBase::prepareConfigDirectories()
- install_ensure_config_directory()
- file_prepare_directory()
- file_uri_scheme()

I guess that adapting or overriding containerBuild can solve the problems here.

7 Failures in core\modules\simpletest\src\Tests\BrokenSetUpTest.php:
Not sure why, but I guess a similar cause as the first 2.

If you do not call parent::setUp(), your test will fail.

There are two problems with that:

BrokenSetUpTest does not call setUp on purpose to show that things are broken.

DrupalKernelTest has this comment:

    // DrupalKernel relies on global $config_directories and requires those
    // directories to exist. Therefore, create the directories, but do not
    // invoke KernelTestBase::setUp(), since that would set up further
    // environment aspects, which would distort this test, because it tests
    // the DrupalKernel (re-)building itself.

Just to see what happens :D

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1005,13 +1005,28 @@ protected function installParameters() {
+    \Drupal::setContainer($this->originalContainer);
...
+    \Drupal::unsetContainer();

+++ b/core/modules/system/src/Tests/DrupalKernel/DrupalKernelTest.php
@@ -36,6 +36,15 @@ protected function setUp() {
+    \Drupal::setContainer($this->originalContainer);
...
+    \Drupal::unsetContainer();

nice.

Wow, BrokenSetUpTest was hard to debug.

Hopefully moving the restoration of the container up a few lines won't break anything.

Okay, so as to not block this on #2363341: Throw exception in Drupal::service() and friends, if container not initialized yet., here's a version of this with those changes stripped out.

Thank for removing 2363341 from this patch. We need BE in IS other then that it is RTBC.

1. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -0,0 +1,507 @@
   + * Contains \Drupal\Core\File\File.
   ...
   +class FileSystem {
   

   File vs. FileSystem ... did you considered adding an interface ... so would you ever want to replace it?

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -0,0 +1,507 @@
   +   * @param $uri
   ...
   +   * @param $mode
   

   meh typehinting.

3. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -0,0 +1,507 @@
   +  protected function getStreamWrapperClass($scheme) {
   +    return file_stream_wrapper_get_class($scheme);
   ...
   +    return file_stream_wrapper_get_instance_by_scheme($scheme);
   ...
   +    return file_stream_wrapper_get_instance_by_uri($uri);
   

   All of those are deprecated

4. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -0,0 +1,507 @@
   +
   +  /**
   +   * Wraps the global Settings singleton.
   +   *
   +   * @codeCoverageIgnore
   +   */
   +  protected function getSetting($name, $default = NULL) {
   +    return $this->settings->get($name, $default);
   +  }
   +
   

   Why do we want to wrap it in the first place?

• Increasing test coverage
• and it could unblock #2304461: KernelTestBaseTNG™ which would make tests run fast, and allow us to write phpunit integration tests (phptests that are not unit tests)
• exposes and fixes inconsistencies and brittleness in several simpletests

adding beta evaluation

html

Addressed the feedback in #55, and updated the issue summary

Exposing more issues, this time in the installer: stream_wrapper_manager is registered as needing the module_handler, which doesn't exist in the early installer, but it actually isn't needed at all.

Awesome!

1. +++ b/core/includes/file.inc
   @@ -10,22 +10,11 @@
    /**
   - * Default mode for new directories. See drupal_chmod().
   - */
   -const FILE_CHMOD_DIRECTORY = 0775;
   -
   -/**
   - * Default mode for new files. See drupal_chmod().
   - */
   -const FILE_CHMOD_FILE = 0664;
   

   Lets' deprecate the constants rather than remove them. Let's say they will be removed in D9.

2. +++ b/core/includes/file.inc
   @@ -129,40 +118,21 @@ function file_stream_wrapper_get_class($scheme) {
   + * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.0.
   + *   Use \Drupal\Core\File\FileSystem::uriScheme().
   

   We should only be saying that new deprecations will be removed in D9. All the deprecations need to be changed.

3. Missing a CR

If we're not going to make any API changes or deprecate anything before D9, why the need for the CR?

I'll fix the other stuff tomorrow, and probably just write the CR as well...

I CRs are valid when we deprecate a load of functions that have been around forever - so people know to use the new functions.

Opened https://www.drupal.org/node/2418133

looks like you got them all.
no unintended changes either.
change record made.

all feedback from @alexpott addressed.
super.

Committed fc63f5e and pushed to 8.0.x. Thanks!

I've updated the beta evaluation and issue summary to reflect the latest changes.