Follow-up from #1653026: [META] Use properly typed values in module configuration.

Problem/Motivation

All integers, Booleans, and even octal numbers in config object files are converted to strings.

Proposed resolution

#1653026: [META] Use properly typed values in module configuration has fixed core, so no need to convert all data types to string anymore.

Remaining tasks

Fix and issue patch for below config files:
editor.routing.yml (just 'TRUE' to TRUE change)

User interface changes

NO

API changes

NO

Parent: #1653026: [META] Use properly typed values in module configuration

Files: 
CommentFileSizeAuthor
#10 editor_cmi_typecasting-2105939-10.patch5.9 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,677 pass(es).
[ View ]
#10 interdiff.txt1.31 KBWim Leers
#7 editor_cmi_typecasting-2105939-7.patch5.9 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,541 pass(es).
[ View ]
#5 editor_cmi_typecasting-2105939-5.patch5.9 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch editor_cmi_typecasting-2105939-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

vijaycs85’s picture

Component:edit.module» editor.module
Wim Leers’s picture

YAY! I remember having to add a bunch of manual typecasting nonsense to editor.module and ckeditor.module's forms to deal with everything being stored as strings. I should now be able to get rid of all that :)

Wim Leers’s picture

Title:Make sure all YML files in Editor module has no type-casting to string.» Make sure all YML files in Editor module has no type-casting to string + config system changes broke image uploading in editors

#1653026: [META] Use properly typed values in module configuration also broke image uploads inside text editors, because of a === '1' check that should now be === 1. The default config file still contains a string, so it will continue to work, but once it's been saved through the UI, it will be an integer, and hence it will fail

webchick’s picture

Priority:Normal» Critical
Issue tags:+Needs tests

Dang. :(

Wim Leers’s picture

Assigned:Unassigned» Wim Leers
Status:Active» Needs review
Issue tags:-Needs tests+sprint, +Spark
StatusFileSize
new5.9 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch editor_cmi_typecasting-2105939-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

#4: I don't think it's reasonable to ask for test coverage of this, the only reason this broke is because of a huge API change (but a good one!). We don't have test coverage to test the results of each trivial setting in Drupal core. It's not needed either: it's overkill. The ROI is too low for cases like these.

I really, really wish the config system used the type metadata in the *.schema.yml files… :(

Status:Needs review» Needs work

The last submitted patch, editor_cmi_typecasting-2105939-5.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new5.9 KB
PASSED: [[SimpleTest]]: [MySQL] 59,541 pass(es).
[ View ]

Chasing HEAD.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

The fact that this was not included with #2106459: Core config has everything as string sounds like also indicates that the yml files in the module don't have config schema coverage? Or were they just missed in some way in #2106459: Core config has everything as string? Other than that:

+++ b/core/modules/editor/editor.admin.inc
@@ -104,7 +105,7 @@ function editor_image_upload_settings_form(Editor $editor) {
-    '#default_value' => $editor->image_upload['max_dimensions']['width'],
+    '#default_value' => ($editor->image_upload['max_dimensions']['width'] === 0) ? '' : $editor->image_upload['max_dimensions']['width'],

@@ -117,7 +118,7 @@ function editor_image_upload_settings_form(Editor $editor) {
-    '#default_value' => $editor->image_upload['max_dimensions']['height'],
+    '#default_value' => ($editor->image_upload['max_dimensions']['height'] === 0) ? '' : $editor->image_upload['max_dimensions']['height'],

Do we need such strict type checking (again)? Is empty() not enough here? Do we really care that much if it was the right type, that we'd rather output '0' if it was a string?

Gábor Hojtsy’s picture

The fact that this was not included with #2106459: Core config has everything as string sounds like also indicates that the yml files in the module don't have config schema coverage? Or were they just missed in some way in #2106459: Core config has everything as string? Other than that:

+++ b/core/modules/editor/editor.admin.inc
@@ -104,7 +105,7 @@ function editor_image_upload_settings_form(Editor $editor) {
-    '#default_value' => $editor->image_upload['max_dimensions']['width'],
+    '#default_value' => ($editor->image_upload['max_dimensions']['width'] === 0) ? '' : $editor->image_upload['max_dimensions']['width'],

@@ -117,7 +118,7 @@ function editor_image_upload_settings_form(Editor $editor) {
-    '#default_value' => $editor->image_upload['max_dimensions']['height'],
+    '#default_value' => ($editor->image_upload['max_dimensions']['height'] === 0) ? '' : $editor->image_upload['max_dimensions']['height'],

Do we need such strict type checking (again)? Is empty() not enough here? Do we really care that much if it was the right type, that we'd rather output '0' if it was a string?

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new1.31 KB
new5.9 KB
PASSED: [[SimpleTest]]: [MySQL] 59,677 pass(es).
[ View ]

#8:

The fact that this was not included with #2106459: Core config has everything as string sounds like also indicates that the yml files in the module don't have config schema coverage

Nope, they do have schema coverage — see editor.schema.yml.

Do we need such strict type checking (again)? Is empty() not enough here? Do we really care that much if it was the right type, that we'd rather output '0' if it was a string?

No, we don't. Strict checks just make it easier to discover subtle problems, IMHO — but I don't feel strongly about that. I've changed it to empty() since you feel that's better.

vijaycs85’s picture

Thanks for working on this. here is a minor question. other than that, +1 to RTBC :)

+++ b/core/modules/editor/editor.admin.inc
@@ -127,5 +128,30 @@ function editor_image_upload_settings_form(Editor $editor) {
+  $form['#element_validate'] = array(
+    'editor_image_upload_settings_validate',
+  );
+
   return $form;
}
+
+/**
+ * #element_validate handler for editor_image_upload_settings_validate().
+ *
+ * Ensures each form item's value is cast to the proper type.
+ *
+ * @see \Drupal\editor\Form\EditorImageDialog
+ * @ingroup forms
+ */
+function editor_image_upload_settings_validate(array $element, array &$form_state) {
+  $cast_value = function($type, $element) use (&$form_state) {
+    $section = $element['#parents'];
+    $value = NestedArray::getValue($form_state['values'], $section);
+    settype($value, $type);
+    NestedArray::setValue($form_state['values'], $section, $value);
+  };
+
+  $cast_value('bool', $element['status']);
+  $cast_value('int', $element['max_dimensions']['width']);
+  $cast_value('int', $element['max_dimensions']['height']);
+}

Do we really need this validation, if we fix type casting issue now?

Wim Leers’s picture

#11: I don't understand the question. The validate handler is specifically and solely being added to fix type casting.

vijaycs85’s picture

sorry @Wim Leers, not very clear there. here is another try :)

if we remove the type casting as part of this issue, do you think still we need this validation to happen? one use case I can think of - use can enter width as '120' (with quotes). IMHO, if there is no width/height, we shouldn't get them saved in config. Saving it as 0 by converting '' (empty) to int seems extra work that we shouldn't do?

for status, we can leave it as what we get?(true/false)?

Wim Leers’s picture

#13: If we don't do this validation, then 1/0 would be stored for status instead of true/false, and '' would be stored instead of 0 for width/height.
A user should not enter width as '120' with quotes.

IMHO, if there is no width/height, we shouldn't get them saved in config.

Well… that's not how most uses of CMI in Drupal core work AFAIK?

Saving it as 0 by converting '' (empty) to int seems extra work that we shouldn't do?

I totally agree. I shouldn't have to do any of the casting, which would mean the entire validate callback would go away. The problem is not editor.module doing this explicitly, the problem is:

I really, really wish the config system used the type metadata in the *.schema.yml files… :(

, which was marked as a follow-up for #1653026: [META] Use properly typed values in module configuration but then never created… :(

Gábor Hojtsy’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

I think this now looks as good as it can for now until a more general schema / form based type casting system emerges. Likely similar things may happen elsewhere in core where new data is written out to YAML.

catch’s picture

Opened #2129399: Use configuration schema for configuration form type casting.

Committed/pushed this to 8.x, thanks!

catch’s picture

Status:Reviewed & tested by the community» Fixed
Wim Leers’s picture

Issue tags:-sprint

Status:Fixed» Closed (fixed)

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