Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sime’s picture

Assigned: Unassigned » sime

Here goes!

sime’s picture

Title: convert file variables to config/state system » convert file.module variables to CMI

Moved file_default_schema to [1799504]

sime’s picture

Assigned: sime » Unassigned

Unassigning, as I got caught up on the #1799504: Convert system file related variables to CMI.

Albert Volkman’s picture

Status: Active » Needs review
FileSize
3.58 KB

First pass at this.

Status: Needs review » Needs work
Issue tags: -Configuration system, -melb-code-sprint-2012-10

The last submitted patch, file_module_variables_cmi-1804394-4.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +melb-code-sprint-2012-10
aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/config/file.settings.ymlundefined
@@ -0,0 +1,2 @@
+description_length: 128
+description_type: 'textfield'
diff --git a/core/modules/file/file.field.inc b/core/modules/file/file.field.inc

diff --git a/core/modules/file/file.field.inc b/core/modules/file/file.field.inc
index 28a32a9..a8c8e28 100644

We should move length and type under description.

description:
- type
- length

icon directory needs to be in here too. And that one probably needs the same structure as above.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
3.63 KB

How's this?

aspilicious’s picture

Status: Needs review » Needs work

I think you forgot to add the update function?

ruth_delattre’s picture

Assigned: Unassigned » ruth_delattre
ruth_delattre’s picture

Status: Needs work » Needs review
FileSize
3.92 KB

Added hook_update_N

Status: Needs review » Needs work

The last submitted patch, file_module_variables_cmi-1804394-11.patch, failed testing.

ruth_delattre’s picture

Assigned: ruth_delattre » Unassigned
ruth_delattre’s picture

Assigned: Unassigned » ruth_delattre
ruth_delattre’s picture

added config-file

ruth_delattre’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, file_module_variables_cmi-1804394-15.patch, failed testing.

ruth_delattre’s picture

Status: Needs work » Needs review
FileSize
4.14 KB

Fixed spelling and indentation errors in yml file

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/file.moduleundefined
@@ -1325,7 +1327,7 @@ function file_icon_url(File $file, $icon_directory = NULL) {
+    $icon_directory =drupal_get_path('module', 'file').'/'.config('file.settings')->get('icon.directory');

"=drupal" needs an extra space. Same for "e').'/'"

ruth_delattre’s picture

Status: Needs work » Needs review
FileSize
4.14 KB

added spaces:
changed
$icon_directory =drupal_get_path('module', 'file').'/'.config('file.settings')->get('icon.directory');
to
$icon_directory = drupal_get_path('module', 'file') .'/'. config('file.settings')->get('icon.directory');

ruth_delattre’s picture

added even more spaces :-/
$icon_directory = drupal_get_path('module', 'file') .'/'. config('file.settings')->get('icon.directory');
is now:
$icon_directory = drupal_get_path('module', 'file') . '/' . config('file.settings')->get('icon.directory');

penyaskito’s picture

RTBC for me.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Oops...

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work

I missed this in my previous review.

+++ b/core/modules/file/file.moduleundefined
@@ -1325,7 +1327,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 = drupal_get_path('module', 'file') . '/' . config('file.settings')->get('icon.directory');
   }

Now we have to hack core if we want a different icon directory that is NOT located in the file module. The correct solution is to hardcode the path in the yml file. Something like "core/modules/file/icons".

ruth_delattre’s picture

Status: Needs work » Needs review
FileSize
4.42 KB

thanx! I wasnt sure which is the better practice but your comment makes complete and utter sense!
Here is the patch with the new path

penyaskito’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/config/file.settings.ymlundefined
@@ -0,0 +1,6 @@
diff --git a/core/modules/file/config/file.settings.yml~ b/core/modules/file/config/file.settings.yml~

diff --git a/core/modules/file/config/file.settings.yml~ b/core/modules/file/config/file.settings.yml~
new file mode 100644
index 0000000..6372c80

index 0000000..6372c80
--- /dev/null

--- /dev/null
+++ b/core/modules/file/config/file.settings.yml~undefined

+++ b/core/modules/file/config/file.settings.yml~undefined
+++ b/core/modules/file/config/file.settings.yml~undefined
@@ -0,0 +1,6 @@

@@ -0,0 +1,6 @@
+description:
+  type: 'textfield'
+  length: 128
+icon:
+  directory: 'icons'

You added the core/modules/file/config/file.settings.yml~ file by accident.
Otherwise, looks fine to me.

ruth_delattre’s picture

Status: Needs work » Needs review
FileSize
4.12 KB

Oops, sorry. Thought I had removed it, but it snug back in. Now, its clean.

ruth_delattre’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

While we're looking at this, does anyone know what these two variables actually exist for?

     $element['description'] = array(
-      '#type' => variable_get('file_description_type', 'textfield'),
+      '#type' => $config->get('description.type'),
       '#title' => t('Description'),
       '#value' => isset($item['description']) ? $item['description'] : '',
-      '#maxlength' => variable_get('file_description_length', 128),
+      '#maxlength' => $config->get('description.length'),

This isn't related to converting them to CMI, but I'm not sure why they exist at all.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Posted a new issue to ask about the variables. Committed/pushed the patch here to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Moved to 1799504