The file systgem settings at admin/config/media/file-system need to be converted to use the new configuration system.

Tasks

  • Create a default config file and add it to the module.
  • Convert the admin UI in system.admin.inc to read to/write from the appropriate config.
  • Convert any code that currently accesses the variables used to use the config system.
  • Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.

This would be a good task for someone wanting to get up to speed on how the new config system works. Feel free to ping me on IRC if you need help.

Files: 
CommentFileSizeAuthor
#145 1496480-file-settings-cmi-145.patch56.41 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 49,094 pass(es).
[ View ]
#145 143-145-interdiff.txt570 bytesalexpott
#142 1496480-file-settings-cmi-142.patch56.53 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedSaveDataTest.php.
[ View ]
#142 1496480-file-settings-cmi-142-interdiff.txt6.48 KBBerdir
#143 1496480-file-settings-cmi-143.patch56.53 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,227 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#143 1496480-file-settings-cmi-143-interdiff.txt660 bytesBerdir
#139 1496480-file-settings-cmi-138.patch52.05 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 49,390 pass(es), 164 fail(s), and 47 exception(s).
[ View ]
#139 interdiff.txt17.42 KBheyrocker
#135 129-135-interdiff.txt4.03 KBalexpott
#135 1496480-135-mi-file_settings-drupal.patch62.46 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 31,503 pass(es), 675 fail(s), and 33 exception(s).
[ View ]
#129 1496480-cmi-file_settings-drupal-129.patch62.46 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#129 1496480-cmi-file_settings-drupal-129-interdiff.txt1.46 KBBerdir
#127 1496480-cmi-file_settings-drupal-127.patch62.16 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#127 1496480-cmi-file_settings-drupal-127-interdiff.txt2.16 KBBerdir
#125 1496480-cmi-file_settings-drupal-125.patch62.19 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#114 file-configuration-cmi-settings-1496480-114.patch62.49 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#106 1496480-cmi-file_settings-drupal-106.patch62.81 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 48,312 pass(es), 215 fail(s), and 90 exception(s).
[ View ]
#103 1496480-cmi-file_settings-drupal-103.patch62.8 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-cmi-file_settings-drupal-103.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#101 1496480-cmi-file_settings-drupal-101.patch60.44 KBACF
FAILED: [[SimpleTest]]: [MySQL] 48,102 pass(es), 230 fail(s), and 53 exception(s).
[ View ]
#99 1496480-99-cmi-file_settings.patch60.44 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#95 1496480-95-cmi-file_settings.patch61.6 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 44,359 pass(es), 512 fail(s), and 564 exception(s).
[ View ]
#93 1496480-92-cmi-file_settings-do-not-test.patch1.6 KBLetharion
#90 1496480-90-files_cmi.patch25.66 KBtyphonius
FAILED: [[SimpleTest]]: [MySQL] 47,218 pass(es), 79 fail(s), and 0 exception(s).
[ View ]
#82 1496480-82-cmi-file_settings.patch9.52 KBLetharion
PASSED: [[SimpleTest]]: [MySQL] 49,301 pass(es).
[ View ]
#77 1496480-77-cmi-file_settings.patch8.76 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#75 1496480-75-cmi-file_settings.patch8.91 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] 49,257 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
#74 1496480-74-cmi-file_settings.patch0 bytesLetharion
PASSED: [[SimpleTest]]: [MySQL] 49,288 pass(es).
[ View ]
#72 1496480-72-cmi-file_settings.patch8.67 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] 49,175 pass(es), 8 fail(s), and 4 exception(s).
[ View ]
#69 1496480-68-cmi-file_settings.patch60.29 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 44,380 pass(es), 466 fail(s), and 492 exception(s).
[ View ]
#66 1496480-65-cmi-file_settings.patch60.31 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-65-cmi-file_settings.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#61 1496480-61-cmi-file_settings.patch59.86 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#59 1496480-59-cmi-file_settings.patch59.73 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-59-cmi-file_settings.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#57 1496480-56-cmi-file_settings-REROLL.patch59.81 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#57 interdiff_1496480-56.txt3.53 KBLetharion
#55 1496480-55-cmi-file_settings-REROLL.patch59.81 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#52 1496480-45-cmi-file_settings-REREROLL.patch61.04 KBcam8001
FAILED: [[SimpleTest]]: [MySQL] 48,018 pass(es), 155 fail(s), and 56 exception(s).
[ View ]
#49 1496480-45-cmi-file_settings-REROLL.patch61.04 KBcam8001
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#45 1496480-45-cmi-file_settings.patch60.96 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-45-cmi-file_settings.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#45 1496480-45-interdiff-do_not_test.patch1.39 KBpfrenssen
#44 1496480-44-cmi-file_settings.patch60.96 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 46,716 pass(es), 155 fail(s), and 56 exception(s).
[ View ]
#41 1496480-41-cmi-file_settings.patch60.96 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 46,714 pass(es), 147 fail(s), and 46 exception(s).
[ View ]
#39 1496480-39-cmi-file_settings.patch60.97 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#33 drupal-file_settings_cmi-1496480-33.patch56.41 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 40,069 pass(es).
[ View ]
#31 drupal-file_settings_cmi-1496480-31.patch56.35 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 39,978 pass(es).
[ View ]
#31 interdiff.txt1.3 KBkbasarab
#27 drupal-file_settings_cmi-1496480-27.patch5.47 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-file_settings_cmi-1496480-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#25 drupal-file_settings_cmi-1496480-25.patch55.35 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#24 23-24-interdiff.txt4.8 KBalexpott
#24 1496480-24-file-settings-cmi.patch54.63 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,750 pass(es).
[ View ]
#23 22-23-interdiff.txt1.22 KBalexpott
#23 1496480-23-file-settings-cmi.patch55.99 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,753 pass(es).
[ View ]
#22 1496480-22-file-settings-cmi.patch55.83 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 39,589 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#22 19-22-interdiff.txt17.17 KBalexpott
#21 1496480-21-file-settings-cmi.patch55.83 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 39,383 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#19 1496480-19-file-settings-cmi.patch56.82 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 39,746 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#19 17-19-interdiff.txt5.51 KBalexpott
#17 1496480-17-file-settings-cmi.patch55.39 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#15 1496480-15.patch84.39 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 1496480-14-do-not-test.patch101.76 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 interdiff.txt687 byteskbasarab
#11 system-filesystemform-cmi-1496-11.patch23.5 KBmrf
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#7 system-filesystemform-cmi-1496480-7.patch22.55 KBmrf
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#5 system-filesystemform-cmi-1496840-5.patch22.55 KBmrf
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.test.
[ View ]
#2 system-filesystemform-cmi-1496480-2.patch4.38 KBmrf
PASSED: [[SimpleTest]]: [MySQL] 35,694 pass(es).
[ View ]

Comments

mrf’s picture

Assigned:Unassigned» mrf
mrf’s picture

StatusFileSize
new4.38 KB
PASSED: [[SimpleTest]]: [MySQL] 35,694 pass(es).
[ View ]

I think I have all the pieces in place, but I'm running into some confusion on how to handle default values that are defined, or partially defined by function calls or other dependencies. I called these out with TODO's in the patch.

Remaining tasks:
Converting current calls throughout the rest of core for these variables
Upgrade path

mrf’s picture

Status:Active» Needs review
mrf’s picture

Status:Needs review» Needs work

Reminder based on discussion that any changeable defaults at install time can be set in the module init don't have to be done via the file unless static. Value will be available to change after that initial set method is called.

mrf’s picture

Status:Needs work» Needs review
StatusFileSize
new22.55 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.test.
[ View ]

Got a lot further along with this one. Still missing the upgrade path, but wanted to see what the testbot has to say about it so far.

Status:Needs review» Needs work

The last submitted patch, system-filesystemform-cmi-1496840-5.patch, failed testing.

mrf’s picture

Status:Needs work» Needs review
StatusFileSize
new22.55 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Fixed the syntax errors, based on running test locally this has introduced quite a few failure point for other systems.

mrf’s picture

beejeebus’s picture

<?
function file_default_scheme() {
- return variable_get('file_default_scheme', 'public');
+ $config = config('system.file_system');
+ return $config->get('file_default_scheme');
}
?>

can be written:

<?php
 
function file_default_scheme() {
   
config('system.file_system')->get('file_default_scheme');
  }
?>

and:

<?php
+    $file_system_config = config('system.file_system');
+   
$file_system_config->set('file_default_scheme', 'temporary');
+   
$file_system_config->save();
?>

this can be written:

<?php
    config
('system.file_system')
      ->
set('file_default_scheme', 'temporary')
      ->
save();
?>

i'm not sure if we have a consensus on chaining yet, but i guess we need one. i'm totally ok with this being decided elsewhere...

marcingy’s picture

mrf’s picture

StatusFileSize
new23.5 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Took another look at this and found an instance where we are looking for file system paths before the config table exists in system.install. I think these missing default values are what was causing the weird test results above.

Not sure what needs to happen to work around this.

Status:Needs review» Needs work

The last submitted patch, system-filesystemform-cmi-1496-11.patch, failed testing.

heyrocker’s picture

I know there are places where we special-case install stuff. For instance, on the system performance form, there is config related to JS and CSS caching that is used in the theme system in the installer before the config table gets created. For instance in drupal_aggregate_css() we have

  // Only aggregate during normal site operation.
  if (defined('MAINTENANCE_MODE')) {
    $preprocess_css = FALSE;
  }
  else {
    $config = config('system.performance');
    $preprocess_css = $config->get('preprocess_css');
  }

So we could do that, special case maintenance mode (which includes install time) and force it to the default value in that case.

kbasarab’s picture

StatusFileSize
new687 bytes
new101.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Test will fail as Installation process fails. But rerolls to had and the interdiff shows a config setting I made to avoid a PHP warning for a missing argument.

kbasarab’s picture

StatusFileSize
new84.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This will fail. Simply rerolling against HEAD and merging the conflicts of test files removing into their respective test files.

sun’s picture

Issue tags:+API change

All config system conversions are API changes, so tagging as such.

alexpott’s picture

Assigned:mrf» alexpott
Status:Needs work» Needs review
Issue tags:-Config novice+D8MI
StatusFileSize
new55.39 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

The patch attached rerolls the patch from #11 as the patches in #14 and #15 have included changes that are not part of this patch.

Currently this patch has issues during upgrade caused due to using config before it's available - which I will hopefully resolve soon. Posting here as a work in progress and to save anyone else who is interested in doing this the reroll.

Additionally the code needs work to improve the comments around what happens during an install.

The patch converts following variables which are present on the file_system_settings form:

  • file_default_scheme
  • file_public_path
  • file_private_path
  • file_temporary_path
  • locale_translate_file_directory (added to the form using a form_alter in the locale module)

It additionally converts the following variables which are part of the file system but can not be configured through the UI.

  • allow_insecure_uploads
  • file_chmod_directory
  • file_chmod_file
  • file_icon_directory

I've removed the novice tag as the installer issues make this conversion quite tricky.

Status:Needs review» Needs work

The last submitted patch, 1496480-17-file-settings-cmi.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new5.51 KB
new56.82 KB
FAILED: [[SimpleTest]]: [MySQL] 39,746 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Fixed the upgrade path failures.

sun’s picture

Status:Needs review» Needs work

Wow, great work so far!

+++ b/core/includes/file.inc
@@ -1126,9 +1127,9 @@ function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXIST
+ * 'system.file.allow_insecure_uploads'. If it evaluates to TRUE, no alterations

@@ -2113,13 +2114,13 @@ function file_get_mimetype($uri, $mapping = NULL) {
+ * This function will use the system.file.permission.directory and
+ * system.file.permission.file configuration for the default modes for

(and possibly elsewhere) Admittedly, we have no standard for this kind of text references to config objects + keys within yet, but in day-to-day issue communication, the syntax of:

module.object:key.subkey

(i.e., delimited by a colon)

...gained some "popularity." For now, I think I'd suggest to go with that. (A dot as delimiter definitely seems wrong to me.)

+++ b/core/includes/file.inc
@@ -2139,10 +2140,16 @@ function file_get_mimetype($uri, $mapping = NULL) {
-      $mode = variable_get('file_chmod_directory', 0775);
+      $mode = intval(config('system.file')->get('permission.directory'), 8);
+      if (!$mode) {
+        $mode = 0775;
+      }
...
-      $mode = variable_get('file_chmod_file', 0664);
+      $mode = intval(config('system.file')->get('permission.file'), 8);
+      if (!$mode) {
+        $mode = 0664;
+      }

@@ -2319,7 +2326,10 @@ function drupal_basename($uri, $suffix = NULL) {
-    $mode = variable_get('file_chmod_directory', 0775);
+    $mode = intval(config('system.file')->get('permission.directory'), 8);
+    if (!$mode) {
+      $mode = 0775;
+    }

1) Hmmmm... there are some really really concerning user comments on intval()'s behavior on http://de.php.net/manual/en/function.intval.php

2) I think we should keep "chmod" as parent key name, since that clarifies that the value actually must be suitable for chmod().

+++ b/core/includes/file.inc
@@ -2402,40 +2412,20 @@ function drupal_tempnam($directory, $prefix) {
function file_directory_temp() {
...
+    $temporary_directory = file_directory_system_temp();

@@ -2444,13 +2434,46 @@ function file_directory_temp() {
+function file_directory_system_temp() {

+++ b/core/lib/Drupal/Core/StreamWrapper/TemporaryStream.php
@@ -19,7 +19,11 @@ class TemporaryStream extends LocalStream {
+      $temporary_path = file_directory_system_temp();

The "system" part confused me a bit in the new function name; especially when I saw it called from elsewhere -- primarily, because there's a system.module. Would it make sense to use "os" instead? Or do we use "system" in other file functions already?

+++ b/core/includes/install.core.inc
@@ -1233,7 +1233,7 @@ function install_find_translations() {
function install_find_translation_files($langcode = NULL) {
-  $directory = variable_get('locale_translate_file_directory', conf_path() . '/files/translations');
+  $directory = conf_path() . '/files/translations';

@@ -1273,7 +1273,7 @@ function install_select_language(&$install_state) {
-        $directory = variable_get('locale_translate_file_directory', conf_path() . '/files/translations');
+        $directory = conf_path() . '/files/translations';

I almost fear that the old variable was supposed to be configurable on a very low-level (e.g., global $conf or some weird distro-level override). We might have to retain that.

+++ b/core/modules/file/file.module
@@ -814,7 +816,7 @@ function file_icon_url(File $file, $icon_directory = NULL) {
-    $icon_directory = variable_get('file_icon_directory', drupal_get_path('module', 'file') . '/icons');
+    $icon_directory = config('system.file')->get('path.icon');

This actually seems to be a File module setting? It doesn't look like it belongs into this conversion?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -617,9 +617,11 @@ abstract class WebTestBase extends TestBase {
     // Set path variables.
-    variable_set('file_public_path', $this->public_files_directory);
-    variable_set('file_private_path', $this->private_files_directory);
-    variable_set('file_temporary_path', $this->temp_files_directory);
+    config('system.file')
+      ->set('path.public', $this->public_files_directory)
+      ->set('path.private', $this->private_files_directory)
+      ->set('path.temporary', $this->temp_files_directory)
+      ->save();

I LIKE! :)

+++ b/core/modules/system/config/system.file.yml
@@ -0,0 +1,14 @@
+inline_types:
+  - ^text/
+  - ^image/
+  - flash$

AFAIK, these belong to File module, too?

+++ b/core/modules/system/system.admin.inc
@@ -1836,18 +1837,36 @@ function system_file_system_settings() {
+  $config = config('system.file');
+  $config

btw, the repeated $config on the second line can be skipped. ->set() returns the config object :)

+++ b/core/modules/system/system.install
@@ -305,26 +305,50 @@ function system_requirements($phase) {
+    if (!empty($GLOBALS['system.file.path.public'])) {

I think you meant

$GLOBALS['conf']['system.file']['path.public']

in all of these?

alexpott’s picture

Status:Needs work» Needs review
Issue tags:-API change, -Configuration system, -D8MI
StatusFileSize
new55.83 KB
FAILED: [[SimpleTest]]: [MySQL] 39,383 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

@sun thanks for the great feedback. Incorporated everything in patch attached.

Do some mode reading on intval() and I've replaced it with octdec() based on http://www.php.net/manual/en/function.chmod.php#99747

alexpott’s picture

Issue tags:+API change, +Configuration system, +D8MI
StatusFileSize
new17.17 KB
new55.83 KB
FAILED: [[SimpleTest]]: [MySQL] 39,589 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Done some mode reading on intval() and I've replaced it with octdec() based on http://www.php.net/manual/en/function.chmod.php#99747

alexpott’s picture

StatusFileSize
new55.99 KB
PASSED: [[SimpleTest]]: [MySQL] 39,753 pass(es).
[ View ]
new1.22 KB

Fix installer language tests and .yml spacing.

alexpott’s picture

StatusFileSize
new54.63 KB
PASSED: [[SimpleTest]]: [MySQL] 39,750 pass(es).
[ View ]
new4.8 KB

In light of #1653026: [META] Use properly typed values in module configuration here's a patch that validates how much nicer this would make our code. The reason this works without the application of a patch is none of the config keys "typed" are maintained by FAPI.

The new YAML File looks like this

allow_insecure_uploads: false
chmod:
    directory: 0775
    file: 0664
default_scheme: public
path:
    private: ''
    public: ''
    temporary: ''

as opposed to

allow_insecure_uploads: '0'
chmod:
    directory: '0775'
    file: '0664'
default_scheme: public
path:
    private: ''
    public: ''
    temporary: ''
disasm’s picture

StatusFileSize
new55.35 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

reroll

sun’s picture

Status:Needs review» Needs work

For now, we need to go back to #23; i.e., string values.

It's probably easiest to reverse-apply the interdiff from #24; e.g.:

git a --reverse 23-24-interdiff.txt

# or alternatively, as git might bail out on invalid path prefixes:
patch -p1 -R -i 23-24-interdiff.txt

disasm’s picture

StatusFileSize
new5.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-file_settings_cmi-1496480-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I think I did this right. I applied 25, reverse applied 23-24-interdiff, and then diff'd out another patch. We'll see if it passes.

disasm’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-file_settings_cmi-1496480-27.patch, failed testing.

disasm’s picture

That patch came out a lot smaller than I expected, and it failed. I'm assuming something went wrong with my process. If I get a chance later this week (maybe office hours on Wednesday) I'll dig into it and see what happened. If someone has free time before then, by all means, have at it.

kbasarab’s picture

Status:Needs work» Needs review
StatusFileSize
new1.3 KB
new56.35 KB
PASSED: [[SimpleTest]]: [MySQL] 39,978 pass(es).
[ View ]

Here's a test at it. I had a similar result to disasm at first. Started QA'ing from the patch and everything looked right. Realized that I had done my diff against the later version instead of 8.x HEAD. Size looks about right now. Interdiff does show an addition I added back for the verbose settings. When I kicked off tests locally the verbose functions weren't found which I traced back to them being removed in patch. You can see what I added back in the interdiff.

alexpott’s picture

Status:Needs review» Needs work

Rerolled patch in #31 looks good - thanks kbasarab!

+++ b/core/modules/system/config/system.file.yml
@@ -0,0 +1,9 @@
+chmod:
+    directory: '0775'
+    file: '0664'

I think we need a YAML comment in here to explain that this should be php octal notation in a string format. Because if a user did this directory: 0700 it would break in a very weird way.

Compare - passing a string to octdec:

$ php -r "mkdir('testing',octdec('0700'));"
$ sudo ls -lah testing
total 12K
drwx------  2 developer developer 4.0K Aug 15 17:09 .
drwx------ 44 developer developer 4.0K Aug 15 17:09 ..

With - passing octal to octdec:

$ php -r "mkdir('testing',octdec(0700));"
$ sudo ls -lah testing
total 12K
d---r--r--  2 developer developer 4.0K Aug 15 17:09 .
drwx------ 44 developer developer 4.0K Aug 15 17:09 ..
kbasarab’s picture

Status:Needs work» Needs review
StatusFileSize
new56.41 KB
PASSED: [[SimpleTest]]: [MySQL] 40,069 pass(es).
[ View ]

Added:

# chmod variables should be a string in PHP Octal Notation.

to line 2 of the yml.

alexpott’s picture

Looks great... I can't rtbc this patch cause I wrote a lot of it... but I think its ready :)

Gábor Hojtsy’s picture

Issue tags:-D8MI

Don't think this belongs with D8MI(?).

aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

reviewed this

catch’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/includes/file.incundefined
@@ -2138,11 +2139,18 @@ function file_get_mimetype($uri, $mapping = NULL) {
+    // Configuration system stores default modes as strings.
     if (is_dir($uri)) {
-      $mode = variable_get('file_chmod_directory', 0775);
+      $mode = octdec(config('system.file')->get('chmod.directory'));

This really needs a comment as to why we're using octdec() vs. intval() or (int). I would've just slapped an (int) or intval() on there too, so a summary of the php docs pages and/or links to them would help people looking at this.

+++ b/core/includes/install.core.incundefined
@@ -1210,9 +1210,23 @@ function install_find_translations() {
+/**
+ * Get installation translations directory path.
+ *
+ * @return
+ *   A string containing the installation translations directory path.
+ */
+function install_translation_directory() {
+  if (isset($GLOBALS['conf']['locale.settings']['path.translations'])) {
+    $directory = $GLOBALS['conf']['locale.settings']['path.translations'];
+  }
+  else {
+    $directory = conf_path() . '/files/translations';
+  }
+  return $directory;
}

We should move all magic globals that aren't configuration overrides out of the 'conf' namespace to somewhere else. But that can happen all at once, doesn't need to be here.

+++ b/core/modules/system/system.installundefined
@@ -305,26 +305,51 @@ function system_requirements($phase) {
+    if ($phase == 'update' && !$config_installed) {
+      // Updating from 7 to 8.
+      // @TODO
+      // Use new functions from http://drupal.org/node/1348162 to load Drupal 7
+      // variables or use Drupal 7 defaults to create $directories array.

hmmm?

pfrenssen’s picture

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

I rerolled the patch against HEAD, converted the newly introduced variables mentioned in #38 and addressed the remarks in #37 (except the $conf issue).

Status:Needs review» Needs work

The last submitted patch, 1496480-39-cmi-file_settings.patch, failed testing.

pfrenssen’s picture

Status:Needs work» Needs review
StatusFileSize
new60.96 KB
FAILED: [[SimpleTest]]: [MySQL] 46,714 pass(es), 147 fail(s), and 46 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1496480-41-cmi-file_settings.patch, failed testing.

pfrenssen’s picture

I just noticed a duplicate of this issue where a ton of work was done: #1799504: Convert system file related variables to CMI. Is it possible to give these people credit for their work on this issue as well?

pfrenssen’s picture

Status:Needs work» Needs review
StatusFileSize
new60.96 KB
FAILED: [[SimpleTest]]: [MySQL] 46,716 pass(es), 155 fail(s), and 56 exception(s).
[ View ]

Interdiff:

--- a/core/modules/locale/lib/Drupal/locale/TranslationsStream.php
+++ b/core/modules/locale/lib/Drupal/locale/TranslationsStream.php
@@ -20,8 +20,7 @@ class TranslationsStream extends LocalStream {
@@ -464,7 +464,7 @@
    function getDirectoryPath() {
-    return variable_get('locale_translate_file_directory',
-      conf_path() . '/files/translations');
-+    return config('locale.config')->get('translation.path');
++    return config('locale.settings')->get('translation.path');
    }
pfrenssen’s picture

Assigned:alexpott» pfrenssen
StatusFileSize
new1.39 KB
new60.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-45-cmi-file_settings.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

There were some more, but the tests are still failing.

Status:Needs review» Needs work
Issue tags:-API change, -Configuration system

The last submitted patch, 1496480-45-cmi-file_settings.patch, failed testing.

rvilar’s picture

Status:Needs work» Needs review

#45: 1496480-45-cmi-file_settings.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API change, +Configuration system

The last submitted patch, 1496480-45-cmi-file_settings.patch, failed testing.

cam8001’s picture

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

How about a reroll?

Status:Needs review» Needs work

The last submitted patch, 1496480-45-cmi-file_settings-REROLL.patch, failed testing.

pfrenssen’s picture

Assigned:pfrenssen» Unassigned
cam8001’s picture

Assigned:Unassigned» cam8001
Status:Needs work» Needs review
Issue tags:-Configuration system
StatusFileSize
new61.04 KB
FAILED: [[SimpleTest]]: [MySQL] 48,018 pass(es), 155 fail(s), and 56 exception(s).
[ View ]

And again.

cam8001’s picture

Issue tags:+Configuration system

Tags lost, putting back.

Status:Needs review» Needs work

The last submitted patch, 1496480-45-cmi-file_settings-REREROLL.patch, failed testing.

Letharion’s picture

Assigned:cam8001» Unassigned
Status:Needs work» Needs review
StatusFileSize
new59.81 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

Rerolling onto HEAD.

Letharion’s picture

Status:Needs review» Needs work

Wow, that was _not_ on HEAD. Sorry.

Letharion’s picture

Status:Needs work» Needs review
StatusFileSize
new3.53 KB
new59.81 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

New re-roll.

Status:Needs review» Needs work

The last submitted patch, 1496480-56-cmi-file_settings-REROLL.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new59.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-59-cmi-file_settings.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Just updating hook_update_N

Status:Needs review» Needs work

The last submitted patch, 1496480-59-cmi-file_settings.patch, failed testing.

vijaycs85’s picture

StatusFileSize
new59.86 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Re-rolling with update change again

vijaycs85’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:-API change, -Configuration system

The last submitted patch, 1496480-61-cmi-file_settings.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review

#61: 1496480-61-cmi-file_settings.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API change, +Configuration system

The last submitted patch, 1496480-61-cmi-file_settings.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new60.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-65-cmi-file_settings.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Adding system.file.xml

Status:Needs review» Needs work

The last submitted patch, 1496480-65-cmi-file_settings.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review

"unable to apply patch"? re-rolling after git pull

vijaycs85’s picture

StatusFileSize
new60.29 KB
FAILED: [[SimpleTest]]: [MySQL] 44,380 pass(es), 466 fail(s), and 492 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1496480-68-cmi-file_settings.patch, failed testing.

Letharion’s picture

*Sigh*. I've tried figuring out the first few errors we're seeing here, and I can't wrap my head around it. Looking back over the issue history, it's clear that a great deal of things have changed during this patches life.

Instead of trying to work backwards with the debugger, I will attempt to "start over" with small parts of the patch, so I can see more exactly what changes cause what fails.

Letharion’s picture

Status:Needs work» Needs review
StatusFileSize
new8.67 KB
FAILED: [[SimpleTest]]: [MySQL] 49,175 pass(es), 8 fail(s), and 4 exception(s).
[ View ]

Cutting scope down to just file_chmod_file and file_chmod_directory. I have odd, seemingly unrelated, test-fails locally. Trying to upload to see what happens.

Status:Needs review» Needs work

The last submitted patch, 1496480-72-cmi-file_settings.patch, failed testing.

Letharion’s picture

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,288 pass(es).
[ View ]
Letharion’s picture

Status:Needs work» Needs review
StatusFileSize
new8.91 KB
FAILED: [[SimpleTest]]: [MySQL] 49,257 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1496480-75-cmi-file_settings.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new8.76 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Updating octal view specification as specified in http://www.yaml.org/refcard.html

Status:Needs review» Needs work
Issue tags:-API change, -Configuration system

The last submitted patch, 1496480-77-cmi-file_settings.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review

#77: 1496480-77-cmi-file_settings.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API change, +Configuration system

The last submitted patch, 1496480-77-cmi-file_settings.patch, failed testing.

typhonius’s picture

Based on http://drupal.org/node/1653026#comment-6311516 they symfony YAML parser converts octals to the decimal equivalent. Therefore perhaps it would either be better to store the permission in the config as a string or cast as an (int) within drupal_chmod.

If the number from the config file is cast to int - the tests pass. I might be missing something vital and trivialising the issue by a simple (int) cast though.

Letharion’s picture

Status:Needs work» Needs review
StatusFileSize
new9.52 KB
PASSED: [[SimpleTest]]: [MySQL] 49,301 pass(es).
[ View ]

What in the world? I was certain I fixed the conversion before I uploaded the patch. Here we go again.
@typhonius Isn't casting to int what we already do with octdec?

typhonius’s picture

Letharion: I may have misinterpreted PHP docs but octdec() appears to convert an octal to its decimal equivalent whereas casting it as (int) appears to take the octal and merely convert the type to int but leave the actual number as is.

I think one of the big differences between #82 and #77 is the permissions being stored as strings vs octal types in the config file. My comment about casting to int was related to permissions not being stored as strings so we're all good with that now.

Your patch seems to have taken care of drupal_chmod and relevant tests in the first instance - I think I'll start to have a look at expanding the patch to cover the scope of other file system settings.

vijaycs85’s picture

nice to see it back in green again :). My only concern is calling octdec() in all places where we do config()->get().

vijaycs85’s picture

Also, wouldn't be quicker if we split them separate as separate task? I made one for file_temporary_path at #1856752: Convert file_temporary_path to the new configuration system

typhonius’s picture

And here's one for file_public_path: http://drupal.org/node/1856766

Berdir’s picture

Not sure that makes sense. Writing the actual code is just a part of what needs to be done, the issues also need to be RTBC'd and commited, and if you split it up, then you will need to re-roll them each time one is commited due to conflicts (upgrade path..).

vijaycs85’s picture

I do agree with not to split, but the number of items are high and failing most cases. Regarding update_N, as it is in system, anyway we need to update when new variable convert. your thoughts?

vijaycs85’s picture

Now I can see the 'Upgrade path' and I'm totally agree with @Berdir in #87. We need to get this issue committed so that we don't need to common stuff(like http://drupal.org/files/17-19-interdiff.txt) in individual patches.

typhonius’s picture

StatusFileSize
new25.66 KB
FAILED: [[SimpleTest]]: [MySQL] 47,218 pass(es), 79 fail(s), and 0 exception(s).
[ View ]

I've had a go at altering almost all of the file_public_path variable_get() calls.

Status:Needs review» Needs work

The last submitted patch, 1496480-90-files_cmi.patch, failed testing.

Letharion’s picture

As far as I can tell, the fails stem from the default value of the file_public_path is set in $conf prior to test installation in WebTestBase. We probably need to move those to new variable-names into $conf as well, but I fail to do so. I could just be doing it wrong, but my attempt with $conf['system.file']['path.public'] = $this->public_files_directory; does not make the value available from config().

Letharion’s picture

Very short patch on top of #82 with debugging code relevant to figure out what's going wrong in #90/#92.

typhonius’s picture

There are a couple of amendments in both TestBase.php and WebTestBase.php that I hoped would address that. I tried doing config()->save on the public files directory location in WebTestBase for example (whilst also leaving it in $conf to try and keep as much working and reduce errors)

When applying the patch locally the affected files pass tests although that's not indicative of a successful patch, rather, that things are roughly on the right track.

Admittedly it took forever for me to realise that to get successful results from config in, say, stream wrappers tests, the correct value for public files location should be saved earlier in the test procedure.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new61.6 KB
FAILED: [[SimpleTest]]: [MySQL] 44,359 pass(es), 512 fail(s), and 564 exception(s).
[ View ]

Trying to cover all the changes for this issue. Locally it is failing on Action looping, but it is not related to this patch (hopefully).

Status:Needs review» Needs work

The last submitted patch, 1496480-95-cmi-file_settings.patch, failed testing.

Letharion’s picture

Action looping was one of the things that didn't make any sense to me and I couldn't figure out, so that's why I tried starting from scratch.

vijaycs85’s picture

Thanks Letharion. I just found this in my local testing:

The requested page "/sites/default/files/simpletest/verbose/Drupal_action_Tests_LoopTest-3.html" could not be found.

It make sense that it's failing on this patch :) Looks like default permission for /files/ is not getting set proper?

vijaycs85’s picture

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

Updating to get current core changes to start work on this patch.

Status:Needs review» Needs work

The last submitted patch, 1496480-99-cmi-file_settings.patch, failed testing.

ACF’s picture

Status:Needs work» Needs review
StatusFileSize
new60.44 KB
FAILED: [[SimpleTest]]: [MySQL] 48,102 pass(es), 230 fail(s), and 53 exception(s).
[ View ]

re-roll to update system.install.

Status:Needs review» Needs work

The last submitted patch, 1496480-cmi-file_settings-drupal-101.patch, failed testing.

vijaycs85’s picture

StatusFileSize
new62.8 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496480-cmi-file_settings-drupal-103.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Tried to test this locally and found that most of the failing test cases are unit Test Cases and the reason is config.factory is not available on PublicStream:

-    return variable_get('file_public_path', conf_path() . '/files');
+    //return config('system.file')->get('path.public');
+    return conf_path() . '/files';

So this code fixed test cases, Updating this hard coded path to make sure this is the only issue for all test cases. if it is then we need to add a fix to make config.factory available in PublicStream::getDirectoryPath().

vijaycs85’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1496480-cmi-file_settings-drupal-103.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new62.81 KB
FAILED: [[SimpleTest]]: [MySQL] 48,312 pass(es), 215 fail(s), and 90 exception(s).
[ View ]

updating patch...

Status:Needs review» Needs work

The last submitted patch, 1496480-cmi-file_settings-drupal-106.patch, failed testing.

heyrocker’s picture

As a heads up, there is talk of removing the public file path from CMI and moving it to the (soon to be) new $settings variable. See #1859110-13: Circular dependencies and hidden bugs with configuration overrides.

chx’s picture

Yes, that's a great idea and absolutely necessary. As pointed out by catch currently the config directory is relative to the public files directory which makes storing the public files directory path inside ... inception.

Berdir’s picture

Is it? I have not yet looked at this closely but if there really would be a dependency, this patch should fail to install/run tests, which is not the case?

Doesn't it just have the same default value logic? Here is the relevant piece from the config_get_config_directory() function

<?php
   
// Allow a configuration directory path to be outside of webroot.
   
if (empty($config_directories[$type]['absolute'])) {
     
$path = conf_path() . '/files/' . $config_directories[$type]['path'];
    }
    else {
     
$path = $config_directories[$type]['path'];
    }
?>

That said, I'm still +1 for putting the public file path into settings but what about private and temporary?

The advantage of having the public file path in settings.php is IMHO that we could then also simplify the config_directories structure and default to absolute = TRUE and remove the nested array structure. So that these two settings would by default overlap but then they'd be in the same file which would make it easier to change them.

chx’s picture

OK, so things do not actually break because we hardwired conf_path() . '/files into config_get_config_directory. I presumed it was using variable_get. If it doesn't, that doesn't really invalidate the argument, it just makes the change from UI a little less devastating but still -- I argue for removing the public files path from the UI because all your uploads still break. So that's not useful. Also, about 99% of the cases the sites/X/files is just fine.

As for private and temporary, it's a whole different bag of hurt. private does not really have a good default as you want to set it outside of docroot. temporary has a default but it's not always working. These two (needs fiddling, no good default) make me vote UI for these. I would put the public path there still with an explanation of why it can't be changed.

Berdir’s picture

As discussed in IRC, there is still a very high chance that users will break their site despite that.

Because after changing the path in the UI, you will move the files directory over the other new location that you configured. And *then* your site will break, because you will have moved the config directory too and the system won't find it anymore.

And that's why I think the absolute option needs to go.

If we have this in settings.php:

<?php
$settings
['file_path_public'] = 'sites/default/files';
$settings['config_directory']['active'] = 'sites/default/files/config/active_aserwesgrsewsef';
?>

(or similar)

then it will be *much* more obvious to anyone who is changing file_path_public that it does in fact not affect the config path and they will either fix that as well or keep the config files where they are.

The next question is then if config should by default be in files but as discussed with @chx, that is a discussion for another day (Has implications on the installation, needs another writable directory).

chx’s picture

$settings['config_directory']['active'] = 'sites/default/files/config/active_aserwesgrsewsef';

+10000000000000000000000000 to make the config directory DRUPAL_ROOT relative instead of the mess we have now.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new62.49 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

updated the patch, replaced path.public with a setting.

Still needs quite a lot of work, just a start.
- There are 25+ definitions of the default value in core, we should either write it into settings.php and require its existence or add a helper function
- Needs a section in settings.php which describes it, similar to other $settings.
- Upgrade path probably needs to write it?
- Fix the failing tests ;)

Status:Needs review» Needs work
Issue tags:-API change, -Configuration system

The last submitted patch, file-configuration-cmi-settings-1496480-114.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+API change, +Configuration system

The last submitted patch, file-configuration-cmi-settings-1496480-114.patch, failed testing.

Berdir’s picture

sun’s picture

With putting the public files path in settings.php, I'm concerned about testing. Web tests in particular.

So far, pretty much everything in settings.php is not testable. And, settings.php force-overrides the environment in which tests are running.

We obviously don't want tests to use, clutter, and destroy the public files path of the parent-site/test-runner. So we'd have to find a discrete way for replacing settings in a web test with settings that are specific to the test environment.

In #1576322-29: page_cache_without_database doesn't return cached pages without the database, @chx explicitly denied writing of a custom settings.php for a test environment that would be used as a replacement for the actual/test-runner's settings.php, due to security reasons; i.e., file paths, authentication credentials, etc. could be exposed to the public.

Berdir’s picture

Hm, yes, I see. That's a problem.

The current patch sets the setting to files/simpletest/... which makes it available within the test class but page requests will just use settings.php, which does not specify it. Had a similar experience with settings.php overrides and tests in 7.x recently.

An idea that I had was that we could *somehow* support a settings.php within the file_storage, which would be a safe thing to write to. That said, I'm not sure how easy it would be to trick Drupal to look there :)

chx’s picture

If we put the whole files url into the agent string instead of just the ID then we could use php storage to read settings.php

sun’s picture

Just to make sure I understand what you guys are thinking of - do you mean something like this?

// Generate:
$test_id = $this->database_prefix;
$php = simpletest_produce_settings_php($test_id);
// Write:
$storage = PhpStorageFactory::get('simpletest');
$storage->save($test_id . '/settings.php', $php);

---

// Read:
if ($test_id = drupal_valid_test_ua()) {
  $storage = PhpStorageFactory::get('simpletest');
  if (!$storage->load($test_id . '/settings.php')) {
    die("Hacks are us.");
  }
}
else {
  require_once conf_path() . '/settings.php';
}

Berdir’s picture

Something along those lines. the problem is that php_storage needs the hash salt, which is defined in settings.php, so we can either only load the test settings.php additionally to the default one or have to pass through the complete file name (or at least the hashed name), so that it can be loaded directly.

Still, not sure if this is even possible, but I don't have any other ideas on how to make that work at the moment :)

And it is a blocker for using settings() for the public file path. I guess with #1887750: Use path relative to DRUPAL_ROOT in configuration directories, it is not actually required anymore, especially if we can move the config directory out of files.

So I'm not sure if we want to go back to the patch in #106 for the time being, and convert it to CMI to get rid of the variable_get() calls as a first step and look into making it a settings() thing later on... ?

heyrocker’s picture

I personally would prefer to do the straight conversion first, then look at the public file path issue as a followup. If we want we can leave the public file path as a variable and convert everything else, to make sure we don't forget about it later.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new62.19 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Ok, let's see if we can get that working.

Straight re-roll of the patch from #106 to see if the installer and so on still work. There are some new variable_get() calls that I haven't look into yet.

Status:Needs review» Needs work

The last submitted patch, 1496480-cmi-file_settings-drupal-125.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new2.16 KB
new62.16 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Fixed the php storage tests, unit test can not use the config system.

Not sure what's up with all those crazy exceptions, let's try again.

Status:Needs review» Needs work

The last submitted patch, 1496480-cmi-file_settings-drupal-127.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.46 KB
new62.46 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Fixed DrupalKernelTest. Still hoping that I'll get to see the results even if there are some weird fatal errors remaining, but those look random to me and usually don't prevent from displaying the results...

Status:Needs review» Needs work

The last submitted patch, 1496480-cmi-file_settings-drupal-129.patch, failed testing.

xjm’s picture

Fatal error: Call to undefined method stdClass::entityType() in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/entity.inc on line 600

stdClass being used for an entity instead of an Entity, looks like?

heyrocker’s picture

Status:Needs work» Needs review

Well, I couldn't reproduce any of these fails locally. Gonna give it another kick but not sure how much it will help :/

heyrocker’s picture

Status:Needs review» Needs work
Issue tags:+API change, +Configuration system

The last submitted patch, 1496480-cmi-file_settings-drupal-129.patch, failed testing.

alexpott’s picture

StatusFileSize
new62.46 KB
FAILED: [[SimpleTest]]: [MySQL] 31,503 pass(es), 675 fail(s), and 33 exception(s).
[ View ]
new4.03 KB

Maybe the patch attached will fix the testbot errors...

I think we should go with heyrocker's suggestion from #124 and remove the public path conversion from this patch... especially since...

+++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.phpundefined
@@ -19,7 +19,8 @@ class PublicStream extends LocalStream {

@@ -19,7 +19,8 @@ class PublicStream extends LocalStream {
    * Implements Drupal\Core\StreamWrapper\LocalStream::getDirectoryPath()
    */
   public function getDirectoryPath() {
-    return variable_get('file_public_path', conf_path() . '/files');
+    //return config('system.file')->get('path.public');
+    return conf_path() . '/files';
   }

I think makes it moot...

alexpott’s picture

Status:Needs work» Needs review
heyrocker’s picture

I've got a patch almost ready that does #124, just doing some testing. Will upload in a bit,

Status:Needs review» Needs work

The last submitted patch, 1496480-135-mi-file_settings-drupal.patch, failed testing.

heyrocker’s picture

Status:Needs work» Needs review
StatusFileSize
new17.42 KB
new52.05 KB
FAILED: [[SimpleTest]]: [MySQL] 49,390 pass(es), 164 fail(s), and 47 exception(s).
[ View ]

OK attached patch removes all conversions of the public file path to config, and reverts them back to variables. This is quite a bit of the patch, but if we can get the rest of the variables converted and done with, I think it makes sense. Lets see how badly I broke everything.

Status:Needs review» Needs work

The last submitted patch, 1496480-file-settings-cmi-138.patch, failed testing.

heyrocker’s picture

Something is going wrong with the translations:// stream, spent a bit of time trying to figure out what but its it's too late.

Berdir’s picture

StatusFileSize
new6.48 KB
new56.53 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedSaveDataTest.php.
[ View ]

So there were two three bugs in WebTestBase.

- The public file path was set to the translation directory. That caused the file field RSS failure.
- The path configuration was written before the modules were installed and then overwritten with the default configuration. So it worked within the test but blow up in the test page.
- The config overrides were wrong, as nested keys need to be an array, not using the . syntax.

Also converted some missing translation path settings and emptied the default path setting and made sure that it's set in locale.install.

Not yet sure what to do about the @todo in the installer but this should pass the tests I think.

EDIT: Forgot one bug, there were three.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new660 bytes
new56.53 KB
FAILED: [[SimpleTest]]: [MySQL] 49,227 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

That set needs a save.

Status:Needs review» Needs work

The last submitted patch, 1496480-file-settings-cmi-143.patch, failed testing.

alexpott’s picture

StatusFileSize
new570 bytes
new56.41 KB
PASSED: [[SimpleTest]]: [MySQL] 49,094 pass(es).
[ View ]

Missing a call to parent::setUp() in a test.

alexpott’s picture

Status:Needs work» Needs review

Dang forgot to change the status...

heyrocker’s picture

Status:Needs review» Reviewed & tested by the community

I think we are good to go here. Will be great to see this get in. I re-opened #1856766: Convert file_public_path to the new settings system (make it not configurable via UI or YAML) to track that progress. Thanks everyone who has worked on this throughout the long issue.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

hass’s picture

Status:Fixed» Closed (fixed)

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

ianthomas_uk’s picture

Status:Closed (fixed)» Active

Reopening since there is still a call to variable_get('stream_public_path', 'sites/default/files'); in core/modules/file/tests/file_test/lib/Drupal/file_test/DummyReadOnlyStreamWrapper.php

Berdir’s picture

Issue tags:-Configuration system

That's just a test variable, let's open a new issue instead of re-opening an old issue with 150 comments.

ianthomas_uk’s picture

Status:Active» Closed (fixed)