Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Related Issues
Parent: #1653026: [META] Use properly typed values in module configuration
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 |
Comments
Comment #1
vijaycs85Comment #2
Wim LeersYAY! 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 :)
Comment #3
Wim Leers#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 failComment #4
webchickDang. :(
Comment #5
Wim Leers#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… :(Comment #7
Wim LeersChasing HEAD.
Comment #8
Gábor HojtsyThe 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:
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?
Comment #9
Gábor HojtsyThe 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:
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?
Comment #10
Wim Leers#8:
Nope, they do have schema coverage — see
editor.schema.yml
.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.Comment #11
vijaycs85Thanks for working on this. here is a minor question. other than that, +1 to RTBC :)
Do we really need this validation, if we fix type casting issue now?
Comment #12
Wim Leers#11: I don't understand the question. The validate handler is specifically and solely being added to fix type casting.
Comment #13
vijaycs85sorry @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)?
Comment #14
Wim Leers#13: If we don't do this validation, then
1/0
would be stored forstatus
instead oftrue/false
, and''
would be stored instead of0
forwidth/height
.A user should not enter width as
'120'
with quotes.Well… that's not how most uses of CMI in Drupal core work AFAIK?
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:
, which was marked as a follow-up for #1653026: [META] Use properly typed values in module configuration but then never created… :(
Comment #15
Gábor HojtsyI 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.
Comment #16
catchOpened #2129399: Use configuration schema for configuration form type casting.
Committed/pushed this to 8.x, thanks!
Comment #17
catchComment #18
Wim Leers