Part of #1775842: [meta] Convert all variables to state and/or config systems meta issue

Variables to convert:
file_chmod_directory
file_chmod_file
file_default_scheme
file_public_path
file_private_path
file_temporary_path

Files: 
CommentFileSizeAuthor
#48 1799504-convert_system_file_related_variables_to_CMI-48.patch44.07 KBCameron Tod
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#48 interdiff.txt282 bytesCameron Tod
#47 1799504-convert_system_file_related_variables_to_CMI-47.patch44.07 KBCameron Tod
PASSED: [[SimpleTest]]: [MySQL] 46,257 pass(es). View
#47 interdiff.txt3.81 KBCameron Tod
#46 1799504-convert_system_file_related_variables_to_CMI.patch44.03 KBandyceo
PASSED: [[SimpleTest]]: [MySQL] 46,196 pass(es). View
#43 drupal-system_file_settings-1799504-43.patch40.81 KBandreiashu
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-system_file_settings-1799504-43.patch. Unable to apply patch. See the log in the details link for more information. View
#41 drupal-system_file_settings-1799504-41.patch40.73 KBandreiashu
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-system_file_settings-1799504-41.patch. Unable to apply patch. See the log in the details link for more information. View
#39 drupal-system_file_settings-1799504-39.patch40.73 KBandreiashu
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#38 drupal-system_file_settings-1799504-38.patch40.72 KBandreiashu
PASSED: [[SimpleTest]]: [MySQL] 42,064 pass(es). View
#35 drupal-system_file_settings-1799504-35.patch50.52 KBandreiashu
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View
#24 drupal-system_file_settings-1799504-24.patch50.97 KBsime
FAILED: [[SimpleTest]]: [MySQL] 42,288 pass(es), 19 fail(s), and 34 exception(s). View
#22 drupal-system_file_settings-1799504-22.patch50.99 KBsime
PASSED: [[SimpleTest]]: [MySQL] 42,302 pass(es). View
#19 drupal-system_file_settings-1799504-19.patch50.48 KBsime
FAILED: [[SimpleTest]]: [MySQL] 41,650 pass(es), 300 fail(s), and 84,200 exception(s). View
#15 drupal-system_file_settings-1799504-15.patch47.68 KBsime
FAILED: [[SimpleTest]]: [MySQL] 41,648 pass(es), 86 fail(s), and 94 exception(s). View
#13 drupal-system_file_settings-1799504-13.patch47.68 KBsime
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View
#12 drupal-system_file_settings-1799504-12.patch47.39 KBsime
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View
#10 drupal-system_file_settings-1799504-10.patch47.29 KBsime
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#9 drupal-system_file_settings-1799504-9.patch29.5 KBsime
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#2 1799504_file_chmod_1.patch8.61 KBandreiashu
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

andreiashu’s picture

Title: Convert file_chmod_directory to CMI » Convert file_chmod_directory and file_chmod_file to CMI

updating title

andreiashu’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1799504_file_chmod_1.patch, failed testing.

andreiashu’s picture

Status: Needs work » Needs review

I think the test bot failed because of: c6bfd262e22778f3681118bd920bcebef7fc515f which was reverted recently.
Retest

andreiashu’s picture

Issue tags: -Configuration system

#2: 1799504_file_chmod_1.patch queued for re-testing.

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

The last submitted patch, 1799504_file_chmod_1.patch, failed testing.

andreiashu’s picture

ok I was wrong. will look into why the installer fails a bit later

sime’s picture

Title: Convert file_chmod_directory and file_chmod_file to CMI » Convert file_chmod_directory, file_chmod_file, file_default_scheme to CMI

Should handle:
file_chmod_directory
file_chmod_file
file_default_scheme

In system.file.yml rather than system.site.yml

sime’s picture

Title: Convert file_chmod_directory, file_chmod_file, file_default_scheme to CMI » Convert system file related variables to CMI
FileSize
29.5 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Issue now covers all these variables:

file_chmod_directory
file_chmod_file
file_default_scheme
file_public_path
file_private_path
file_temporary_path

Latest patch attached, but not complete. Todo:

-- convert file_*_path variables
-- update system_file_system_settings() to use system_config_form and provide a submit handler.

sime’s picture

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

TODO:

1) Update system_file_system_settings() to use system_config_form() and provide a submit handler.

2) Some settings uses conf_path() to define the default. I do not know how to define this in the yml file. I am currently doing:

path:
  public: 'sites/default/files'
  private: ''
  temporary: 'sites/default/files/tmp'

Status: Needs review » Needs work

The last submitted patch, drupal-system_file_settings-1799504-10.patch, failed testing.

sime’s picture

FileSize
47.39 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View

I was apparently unable to do "file mode" as a octal in the YAML file. So I used integers.

# Directory and file modes.
# 0775 is 509, as converted from octal, and 664 is 436.
chmod:
  directory: 509
  file: 436

There are still errors in the installation.

sime’s picture

Status: Needs work » Needs review
FileSize
47.68 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View

Configuration should set temporary file directory as NULL

Status: Needs review » Needs work

The last submitted patch, drupal-system_file_settings-1799504-13.patch, failed testing.

sime’s picture

Status: Needs work » Needs review
FileSize
47.68 KB
FAILED: [[SimpleTest]]: [MySQL] 41,648 pass(es), 86 fail(s), and 94 exception(s). View

Fixed install hook conflict.

Status: Needs review » Needs work

The last submitted patch, drupal-system_file_settings-1799504-15.patch, failed testing.

sime’s picture

Yay, got past the installer. Working on the system form.

sime’s picture

Assigned: andreiashu » sime

I should probably take ownership of the issue to avoid confusion.

sime’s picture

FileSize
50.48 KB
FAILED: [[SimpleTest]]: [MySQL] 41,650 pass(es), 300 fail(s), and 84,200 exception(s). View

The system settings form is done, not all tests are passing though.

sime’s picture

Status: Needs work » Needs review

My local tests against an 8.x branch are giving me fails so going to see what the testbot says.

Status: Needs review » Needs work

The last submitted patch, drupal-system_file_settings-1799504-19.patch, failed testing.

sime’s picture

Status: Needs work » Needs review
FileSize
50.99 KB
PASSED: [[SimpleTest]]: [MySQL] 42,302 pass(es). View

Fixed up ->drupalGetTestFiles()

Status: Needs review » Needs work

The last submitted patch, drupal-system_file_settings-1799504-22.patch, failed testing.

sime’s picture

Status: Needs work » Needs review
FileSize
50.97 KB
FAILED: [[SimpleTest]]: [MySQL] 42,288 pass(es), 19 fail(s), and 34 exception(s). View

OK, 1 exception left. I do not know how to prevent it properly. It occurs in MTimeProtectedFileStorageTest.php in the testSecurity() method.

$expected_root_directory = DRUPAL_ROOT . '/' . config('system.file')->get('path.public') . '/php/simpletest';

I do not know why config('system.file')->get('path.public') is returning nothing when it is working fine in other tests. Also these PHPStorage tests are badly broken for me whereas the testbot shows only one exception.

Looking at the code, the old code uses a variable get and doesn't care much if it needs to grab a default value comprised of conf_path() . /files, so the new patch *might* be appropriate, and replaces the above line with:

$expected_root_directory = DRUPAL_ROOT . '/' . $this->originalFileDirectory . '/php/simpletest';
sime’s picture

Assigned: sime » Unassigned

Otherwise, this is pretty close... unassigning myself, but will keep an eye on it.

Status: Needs review » Needs work

The last submitted patch, drupal-system_file_settings-1799504-24.patch, failed testing.

sime’s picture

Looks like #22 is the best effort so far.

andreiashu’s picture

Patch form #22 works fine for me and all PHPStorage tests are green so retesting

andreiashu’s picture

Status: Needs work » Needs review
Dave Reid’s picture

This looks perfectly sane except for the removal of file_default_schema(), which could be changed to be a wrapper for config(), much like file_directory_temp(). Let's please revert those removals of file_default_schema() in the name of DX.

diff --git a/core/modules/image/image.field.inc b/core/modules/image/image.field.inc
index 09b07eb..3a8b0f9 100644
--- a/core/modules/image/image.field.inc
+++ b/core/modules/image/image.field.inc
@@ -14,7 +14,7 @@ function image_field_info() {
       'label' => t('Image'),
       'description' => t('This field stores the ID of an image file as an integer value.'),
       'settings' => array(
-        'uri_scheme' => variable_get('file_default_scheme', 'public'),
+        'uri_scheme' => config('system.file')->get('default_scheme'),
         'default_image' => 0,
       ),
       'instance_settings' => array(

This should use file_default_scheme() instead.

diff --git a/core/lib/Drupal/Core/StreamWrapper/TemporaryStream.php b/core/lib/Drupal/Core/StreamWrapper/TemporaryStream.php
index 52a6321..fd23619 100644
--- a/core/lib/Drupal/Core/StreamWrapper/TemporaryStream.php
+++ b/core/lib/Drupal/Core/StreamWrapper/TemporaryStream.php
@@ -19,7 +19,12 @@ class TemporaryStream extends LocalStream {
    * Implements Drupal\Core\StreamWrapper\LocalStream::getDirectoryPath()
    */
   public function getDirectoryPath() {
-    return variable_get('file_temporary_path', file_directory_temp());
+    $temporary_path = config('system.file')->get('path.temporary');
+    if (empty($temporary_path)) {
+      // Determine a temporary path. This will save the value to config for next time.
+      $temporary_path = file_directory_temp();
+    }
+    return $temporary_path;
   }
 
   /**

This should just be using return file_directory_temp() as a cleanup.

andreiashu’s picture

Thanks Simon, patch from #22 is now green. Can someone review and RTBC please?
Credits for the patch should go to Simon please

Dave Reid’s picture

I guess from a security perspective, are the following comments preserved in the exported config files? Otherwise I could see people trying to manually change these values to '0775' and things not working as expected.

+# Directory and file modes.
+# 0775 is 509, as converted from octal, and 664 is 436.
andreiashu’s picture

Status: Needs review » Needs work

@Dave: oops posted at the same time.
will amend

Dave Reid’s picture

heyrocker confirms comments are not preserved, so based on #32 alone I need to mark this as needs work.

andreiashu’s picture

FileSize
50.52 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View

Attached patch a per Dave's suggestions in #30 and #34

andreiashu’s picture

oops. Ignore previous patch, I need to use file_default_scheme() in all places where we were using config directly...
new patch soon to come

Dave Reid’s picture

A little confused why we can't actually use octal in YAML? http://trac.symfony-project.org/ticket/7067 seems it was fixed three years ago?

andreiashu’s picture

Status: Needs work » Needs review
FileSize
40.72 KB
PASSED: [[SimpleTest]]: [MySQL] 42,064 pass(es). View

Patch rerolled and updated per Dave's review. If we get green tests I'll attach another one with the octal values

andreiashu’s picture

FileSize
40.73 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

This one is with octal in YAML file

Status: Needs review » Needs work

The last submitted patch, drupal-system_file_settings-1799504-39.patch, failed testing.

andreiashu’s picture

Status: Needs work » Needs review
FileSize
40.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-system_file_settings-1799504-41.patch. Unable to apply patch. See the log in the details link for more information. View

patch from #39 had the perms as strings in the config file.
Attached patch has proper octal values in config this time

Status: Needs review » Needs work

The last submitted patch, drupal-system_file_settings-1799504-41.patch, failed testing.

andreiashu’s picture

Status: Needs work » Needs review
FileSize
40.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-system_file_settings-1799504-43.patch. Unable to apply patch. See the log in the details link for more information. View

rerolled

andyceo’s picture

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

The last submitted patch, drupal-system_file_settings-1799504-43.patch, failed testing.

andyceo’s picture

Status: Needs work » Needs review
FileSize
44.03 KB
PASSED: [[SimpleTest]]: [MySQL] 46,196 pass(es). View

Re-roll of patch in #43 comment.

Cameron Tod’s picture

FileSize
3.81 KB
44.07 KB
PASSED: [[SimpleTest]]: [MySQL] 46,257 pass(es). View

Reroll for current head.

Cameron Tod’s picture

FileSize
282 bytes
44.07 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Changing octals back to strings after a discussion here at the London drop-in sprint.

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

The last submitted patch, 1799504-convert_system_file_related_variables_to_CMI-48.patch, failed testing.

Cameron Tod’s picture

Status: Needs work » Needs review

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

The last submitted patch, 1799504-convert_system_file_related_variables_to_CMI-48.patch, failed testing.

deviance’s picture

Assigned: Unassigned » deviance
deviance’s picture

Assigned: deviance » Unassigned
pfrenssen’s picture

When I apply this patch and try to install Drupal using the Standard profile through the interface I get the following error:

The service definition "keyvalue" does not exist.

pfrenssen’s picture

Status: Needs work » Closed (duplicate)

This is unfortunately a duplicate of #1496480: Convert file system settings to the new configuration system :(

I'll propose in the original issue that commit credit is given to the people that worked on it here so that the hard work done is not entirely lost.

pfrenssen’s picture

Issue summary: View changes

Updated issue summary to include all variables.