Make sure all YML files in Editor module has no type-casting to string + config system changes broke image uploading in editors [#2105939]

Comment	File	Size	Author
#10	editor_cmi_typecasting-2105939-10.patch	5.9 KB	Wim Leers

#10	interdiff.txt	1.31 KB	Wim Leers
#7	editor_cmi_typecasting-2105939-7.patch	5.9 KB	Wim Leers

#5	editor_cmi_typecasting-2105939-5.patch	5.9 KB	Wim Leers

Comment #1

vijaycs85

London, UK

CreditAttribution: vijaycs85 commented 6 October 2013 at 18:59

Component:

edit.module

» editor.module

Log in or register to post comments

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 7 October 2013 at 15:17

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 :)

Log in or register to post comments

Comment #3

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 10 October 2013 at 15:25

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

Log in or register to post comments

Comment #4

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 10 October 2013 at 19:42

Priority:	Normal	» Critical
Issue tags:		+Needs tests

Dang. :(

Log in or register to post comments

Comment #5

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 28 October 2013 at 14:43

Assigned:	Unassigned	» Wim Leers
Status:	Active	» Needs review
Issue tags:	-Needs tests	+sprint, +Spark

File	Size
editor_cmi_typecasting-2105939-5.patch	5.9 KB

#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… :(

Log in or register to post comments

Comment #6

28 October 2013 at 14:45

Status:

Needs review

» Needs work

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

Log in or register to post comments

Comment #7

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 28 October 2013 at 14:49

Status:

Needs work

» Needs review

File	Size
editor_cmi_typecasting-2105939-7.patch	5.9 KB

Chasing HEAD.

Log in or register to post comments

Comment #8

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 28 October 2013 at 15:00

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?

Log in or register to post comments

Comment #9

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 28 October 2013 at 15:00

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?

Log in or register to post comments

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 28 October 2013 at 15:11

Status:

Needs work

» Needs review

File	Size
interdiff.txt	1.31 KB
editor_cmi_typecasting-2105939-10.patch	5.9 KB

#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.

Log in or register to post comments

Comment #11

vijaycs85

London, UK

CreditAttribution: vijaycs85 commented 28 October 2013 at 16:06

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?

Log in or register to post comments

Comment #12

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 28 October 2013 at 16:08

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

Log in or register to post comments

Comment #13

vijaycs85

London, UK

CreditAttribution: vijaycs85 commented 28 October 2013 at 16:22

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)?

Log in or register to post comments

Comment #14

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 28 October 2013 at 16:47

#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… :(

Log in or register to post comments

Comment #15

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 6 November 2013 at 11:51

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.