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.
Problem
- Lots of integers, Booleans, and even octal numbers in config object files, but all of them are converted to strings.
Goal
- Research whether it is feasible to remove the enforced type-casting to strings for all config values.
Details
- The reason we are force-casting everything into a string is because that is the common denominator for all possible storage engines.
- There are considerations to change the active store implementation to essentially store each key within the config object as a separate item/record.
- However, right now, we're dealing with YAML and serialized PHP. Both of them support primitive data types.
- For modules that have to work on other data types than strings, we're implanting lots of data type conversions currently.
Proposed solution
- Remove the enforced type-casting to strings.
- Update all config files accordingly.
Notes
- This destroys native support for alternative serialization formats, such as JSON and XML.
Related Issues
- #1945246: Can Symfony .yml exporter export integer/boolean values *without* quotes in configuration files? marked as a duplicate of this issue.
- @webchick's comment on #1938570-11: Make views active config save format match the default yml file (order and quotes)
- #1930226: Update integer/boolean values with quotes in configuration files. (when we fix how the config is exported, we'll have to make the default match again)
- We may have to change http://drupal.org/node/1905070 : the d.o doc page pointed to from the change record (http://drupal.org/node/1905120) from the issue #1866610: Introduce Kwalify-inspired schema format for configuration
Original report:
During the discussions around #1650778: Convert default views into YAML files for CMI we've come up against the issue of data typing. Yaml does support preserving types when saving. However, currently the DrupalConfig
object casts all values to strings. Should we do this?
In my opinion config should be a dumb as possible - which includes returning exactly what was given. So if given the data:
array(
'test' => 2,
3 => 1,
);
We should return an identical array.
Currently we return
array(
'test' => '2',
3 => '1', // Yes key the remains numeric!
);
Sub-tasks #
Sub-tasks completed#
Comment | File | Size | Author |
---|---|---|---|
#149 | config-1653026-149.patch | 69.39 KB | krishnan.n |
#146 | config-1653026-146.patch | 326.51 KB | krishnan.n |
#124 | config-strings-1653026-124.patch | 23.93 KB | tim.plunkett |
#124 | interdiff.txt | 761 bytes | tim.plunkett |
#122 | config-1653026-122.patch | 23.95 KB | tim.plunkett |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedAfter coming accross this whilst starting CMI integration for Views (above issue), this makes alot of sense to me. Config should be as you say, 'dumb', and return exactly what it is given to store IMO. The YAML spec deals with types, so I think we should use them.
Comment #2
sunThe reason we are force-casting everything into a string is because that is the common denominator for all possible storage engines.
(FYI: There are considerations to change the active store implementation to essentially store each key within the config object as a separate item/record.)
Comment #2.0
sunFixed code comment on second array
Comment #3
sunI'm bumping this to a major bug, since we should better be sure that we'll go with the enforced type-casting, as we're implanting more and more type-casts into runtime code in the current config conversion patches.
Due to that, I've also replaced the issue summary.
Comment #4
alexpottCouldn't resist rolling a patch to do part 1 of proposed solution.
This additionally solves the issue of config's
setData($array)
method not being equivalent toset($key, $value)
method. A config object created using the setData currently does not have the values cast to strings.Comment #5
alexpottCrosspost... :(
Comment #6
sunComment #7
sunThe important part is that every storage controller needs to be able to support the same features.
But since FileStorage uses YAML and DatabaseStorage uses serialized PHP variables, that requirement would be met (for the implementations we have in core).
To confirm that the system works as intended, this patch needs to update ConfigStorageTestBase accordingly; i.e., assert that integers, floats, Booleans, octal numbers, etc are written and read as expected.
Comment #7.0
sunUpdated issue summary.
Comment #8
sunI think we want to do this.
However, we need rigorous tests which assert that we do _not_ support insane data types (such as a serialized PHP class instance or resource...)
Comment #9
alexpottHave a look at http://drupal.org/node/1496480#comment-6293186 for what this change will gives us.
One issue at the moment is that many of the config conversion YAML files are being produced by hand. Many of the elements set by FAPI will still have to be strings (eg. image style width and height 'numbers') since if these are changed by the UI they will become strings. Additionally I think a single checkbox returns an integer of 0 or 1 not a 'true' boolean.
So the question could be: should we be type-casting on input? I think this might be a good option as it'll make reads more performant and we shouldn't really care too much about writes since if theses are happening on every page request we/you are abusing the configuration system.
re #8: I'm working on those tests.
Comment #10
alexpottWith some tests - we still have more to write as we need to test setting configuration with types using a config object rather than the one of the Storage implementations.
Comment #11
alexpottMissing patch...
What is quite amusing is that we had tests for casting a boolean to a string equaling '0' or '1'.
They don't work :)
From ConfigFileContentTest.php
I think
$this->assertIdenitcal
should have been used - gotta love PHP :)Comment #12
sunI'd prefer to leave ConfigFileContentTest alone... this test was the first that was added and basically exists for legacy reasons only. It has no clear scope and purpose, and we should remove it at some point.
Technically, storage controllers are responsible for encoding and decoding, so this change mainly affects storage controllers only, so the entire expectations and assertions should happen in ConfigStorageTestBase. (It is also the StorageInterface::decode() method that will blow up when the data contains e.g. a serialized PHP object that cannot be re-instantiated.)
However, I'm not sure where we want to implant the data type validation (to disallow PHP objects and resources from getting serialized). It might make sense to do that in Config::set() -- although, as you've pointed out already, Config::setData() would then be able to circumvent that validation. So perhaps the validation might need to happen in StorageInterface::encode()...?
The data type validation I have in mind is:
Comment #13
sunSomething along the lines of this. Please consider this a quick hack and prototype only.
Comment #15
alexpottThis patch extends @sun's idea by implementing a new method
StorageInterface::validateData()
.This allows better error messages along the lines of "Drupal\Core\Config\StorageException: Unallowed non-scalar value of type object in key config_key in config config_object". Additionally there's no point in trying to print the value of an object so now the error message just prints the type of the invalid value.
Comment #16
alexpottLets test :)
Comment #17
alexpottAdded tests for preserving and validating data types using config object.
Comment #18
sunA procedural callback in config.inc doesn't really fly for me, given that we just did #1704196: Remove Config's dependencies on procedural Drupal code in includes/common.inc
So even though the location is still wonky, I'm moving the callback back onto Config.
Also, array_merge_recursive() supports additional user parameters to be supplied to the callback, so the indirection and duplicate exception handling is unnecessary. ;)
Comment #20
alexpottHow about bringing back the abstract class StorageBase? There's never a wrong idea... just the wrong time :)
Also I'm a fan of creating a validateData method since we might want to validate data and not write it... eg. maybe on import... you never know.
Comment #21
alexpottGo testbot go...
Comment #22
alexpottRemove unnecessary duplicate validateData() method in config object.
Comment #23
alexpottRemove unnecessary static - In fact the statics on encode and decode are no longer necessary (will raise an issue later) - our design must be getting better :)
Comment #24
sunHrm.
1) In my mind, a long-term is still to rebase the config storage controllers onto #1202336: Add a key/value store API, as soon as that is in core. Making all storage controllers extend from a base controller seems incompatible with that.
2) While working on an indentation fix for the Yaml\Dumper, I recognized that it in fact has built-in support for serialized objects: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Yam... -- but even with that, I'm not sure whether config system should support that. I don't think so.
3) It might make most sense to move the validation into the Config::save() method. Since that is our high-level layer that contains the business logic for operating on config objects in the end.
Comment #25
gddI agree with sun that Config::save() is probably the best place to be doing this. Are we concerned about there being some inconsistency in the files due to the issues with data coming from FAPI? For instance, booleans being stored as true/false in some places and '0'/'1' in others. That kind of bothers me.
One thing that could make this nicer is if we renamed the keys of boolean values so they are a little more self-describing (aka 'cache_enabled' vs 'cache') but that's probably fodder for another issue.
Comment #26
sunThe Form API values part is actually the most concerning for me here. Forgot to mention that somehow, sorry.
Problem being, Form API expects string input all over the place. There can be #default_value/#value_callback implementations that reject and straight-out deny non-string values, which makes _form_validate() ultimately blow up with the most awful error message of
E.g., see #811542: Regression: Required radios throw illegal choice error when none selected
For some reason, this patch doesn't seem to trigger that validation error yet - but it's possible that this is only the case, because we're still converting stuff to config, and the config we currently have might not contain edge-cases yet.
Comment #27
alexpottMoved validation logic into config object.
Maybe the reason this patch hasn't trigger the validation error yet is because the config defaults are all strings :)
Comment #28
sunThis looks much better now.
Ultimately, however, the consequences are still a bit blurry.
I can only suspect that - if the chmod file settings were configurable through the UI and thus Form API - then we'd have to do some data type munging in the form constructor and handlers, so as to convert the original data type into one that is compatible for Form API, and on submit it would have to be converted from whatever input/string back into the config data type.
Comment #30
gddYes, sun is right, I spent some time playing around with this. For instance,
octal: 0755
in a YAML file will get converted to 493 by the Symfony YAML parser and thus printed as such in a form. You would actually need to somehow know in advance that you were dealing with an octal value at read and save time in order for data to be dealt with properly. I suppose we could add type-specific FAPI textfields that can handle the conversions behind the scenes ('#type' => 'octal'
woulddecoct()
default values on read andoctdec()
them on submit, same for booleans in checkboxes aka false->0->false) but that is going to get super-ugly and is a much bigger change.I'm not sure there's a good way for us to fix this without going through a lot of additional pain.
Comment #31
sunSo... the essential assertion that is missing in this patch is that the YAML file actually still contains the data types after writing them, because assertIdentical() only asserts this:
The moment we'd write 493 to the config file, the configuration value would lose its human-readable meaning. And this already happens right at installation time, when the default config files are loaded from the module's config dir and saved to the site's config dir.
The Yaml tests actually clarify that our expectations are incompatible with the YAML specification and/or the PHP interpreter.
There also was a very interesting discussion about this on yaml's mailing list some years ago - somewhat starting with the same symptom, but then continuing to an even brainfsck example of a
06511
zipcode ;)http://comments.gmane.org/gmane.text.yaml.general/2689
It looks like the only way to resolve this would be via metadata - i.e., whenever a config file is written, run its data against a file-specific schema, so as to convert the data types before they are written.
Comment #32
gddThe zip code example is pretty entertaining. I guess we need to postpone this on #1648930: Introduce configuration schema and use for translation at the very least.
Comment #33
gddI'm also de-prioritizing.
Comment #34
sunIf we go with the simplified plan in #1324618-91: Implement automated config saving for simple settings forms, then we could actually make a proper "form input to config data type" conversion a part of the new tutorial for "final-converting" settings forms into config forms.
I.e., a manual conversion, along the lines of:
The remaining edge-cases of #31 would have to be documented.
In other words, I'm still +1 on this and would like to do it. The entire whatever-data-type-to-string futzing turned out to be a major confusion for most people involved in the config conversion issues already, so getting rid of it would be a major help and benefit -- even if it introduces some rare edge-cases at the same time.
Comment #35
sun[hackfest] Re-rolled against canonical file storage branch. Will pass when that is in.
Comment #36
sunIntroducing a new YAML issue tag to track the issues with it.
Merged in HEAD.
Removed the custom validation for data types in Config entirely to allow usage of objects.
Adjusted tests accordingly.
Also removed the config file conversions for now, since there have been too many conflicts.
Comment #38
webchickReading patches like #1938570-8: Make views active config save format match the default yml file (order and quotes) make me sad, and I'm told this issue is at fault.
It seems like this should be a major bug, rather than a normal one, since it's quite the WTF for someone to define proper boolean/integer/etc. values in their YAML files like they can in any other framework and then have them unceremoniously dumped out as strings.
Comment #39
alexpottThis issue can go some way fix the quoting... the order is something that needs to be "right"...
Comment #40
DamienMcKennaWould it make sense to split this into separate issues: the ability to correctly read files valid to the YAML specification so we don't create Yet Another Drupalism, and the ability to correctly wrote/export non-Drupalism YAML?
Comment #41
DamienMcKennaFYI my rationale for requiring D8 to accept valid YAML, rather than Drupal YAML, is that it will make it much easier for other YAML-capable tools to integrate with Drupal without needless "Oh, Drupal doesn't like this perfectly clean YAML" hackery.
Comment #42
webchickHm. I don't think "Drupalism" applies here... this isn't a case of our own special flavour of YAML, it's a case of "What goes in isn't what you get out." It's just a straight-up bug IMO.
Comment #43
YesCT CreditAttribution: YesCT commented#1945246: Can Symfony .yml exporter export integer/boolean values *without* quotes in configuration files? marked as a duplicate of this. :) Just linking for completeness.
Comment #43.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #43.1
YesCT CreditAttribution: YesCT commentedadded duplicate and related issues
Comment #44
vijaycs85hope #1944636: PECL Intl extension usage in DateTimePlus class broken and untested is related to this issue...
Comment #45
alexpottNope #1944636: PECL Intl extension usage in DateTimePlus class broken and untested is nought to do with this.
Comment #46
damiankloip CreditAttribution: damiankloip commentedRerolled @sun's patch, as it's been nearly 6 months and surprisingly, didn't apply cleanly :)
I also removed the tests for object types, as I don't think we are entertaining that idea anymore?
Also if we want the last assertion in COnfigCRUDTest::testDataTypes to pass, I think we need to change this line in FileStorage::encode:
Comment #48
damiankloip CreditAttribution: damiankloip commentedSorry, forgot the test yaml file, also me, alexpott, and beejeebus discussed a couple of changes in IRC. Removing the foreach for invalid types in the test and add an integer as a string to the tests.
Comment #49
damiankloip CreditAttribution: damiankloip commentedAnd the newline...
Comment #51
damiankloip CreditAttribution: damiankloip commentedThis should fix up the test failures, now we have proper type values.
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedthis is looking good.
i'd like to see us check the strings on disk as well as in-memory data here, because config->save() followed by config->get() on the same object will just give you back the in-memory values, and not refetch from disk.
so, can we test the strings on disk are what we expect as well as the in-memory values? or reload the object then test ->get()? or both?
Comment #53
gddI don't have anything against this patch except that it doesn't help anything at all because we're still going to have Form API dumping crap into our files. Here's an example. I applied this patch and then took system.performance.yml and converted it to use proper boolean and int values:
Now here is what happens by simply opening the Performance settings form and saving it back with no changes
Note that all the bools have been changed to 0/1, and the int max_age has been quoted.
While I agree that making everything in the YAML files as strings is somewhat of a 'Drupalism', it also means our files are consistent and it is very easy to teach people how to write them. 'Quote everything. The end.' Otherwise we have to figure out what Form API is going to write in every single possible situation and document it to make them match, and even then it won't be 'proper' YAML. See also the discussion above about the difficulty of determining what format something should be for data like octals and zip codes.
Our only hope for a real solution here is to add data types or config-data binding to Form API and its just too late for those types of changes.
Amateescu did have an idea in IRC earlier, when we save config data, we could check against the schema, get the type, and cast accordingly. This could work, although it would require some mapping to base types (for instance, type: email doesn't help unless we know email is a string or alternately just assume anything we don't recognize is a string.) I could potentially get on board with this type of change, however I think the patch as it currently stands is a no-go because its just going to introduce confusion.
Comment #54
gddAlso note that schema are totally not going to be required, and I'm sure many contrib won't provide them, so relying on it for file consistency will bring us back to square one in those situations (which we may not care about but its worth bringing up.)
Comment #55
amateescu CreditAttribution: amateescu commentedContrib usually follows core and other (major) contrib, so as long as we set some good examples and documentation, I think we should be good. Or they can be fixed :/
Edit: actually, I got that order wrong, major contrib is first because core can be quite daunting to look into at first.
Comment #56
damiankloip CreditAttribution: damiankloip commentedI can see the concerns here, but I really think we shouldn't limit the configuration system based on the form api. The configuration system should be about storing the data, imo it shouldn't be making this casting assumption at all. If we want to give people more direction as to how they should create these files, so be it.
I just get the feeling that leaving this in will bite us at some point down the line. There may be certain implementations using config that don't use the form api to save things, and therefore, would not benefit at all by this.
Comment #57
amateescu CreditAttribution: amateescu commentedErr.. what? My proposal was to cast based on the config schema, form api is not involved anywhere in this?
Comment #58
damiankloip CreditAttribution: damiankloip commentedI'm just saying generally, one of the main arguments not for is the form api. Until I see your plan in action....
Comment #59
damiankloip CreditAttribution: damiankloip commentedJust rerolling and including feedback from #52.
Comment #60
damiankloip CreditAttribution: damiankloip commentedRerolled, didn't apply.
Comment #61
alexpottSo I think that we're breaking an important principle here... that config should return the data in an identical format to what is passed to config->save(). At Barcelona @merlinofchaos said that CMI should be a dumb as possible and I think that here we are breaking that maxim.
Just because FAPI returns everything as strings is not a reason to go ahead here. We can leave all the simple config as strings if we want or we can use the schema (if available) to save typed config. But config entities should be allowed to save their data in the types stored on the object.
This is in my opinion rtbc - assigning to @heyrocker cause we need his buy-in.
Comment #62
Gábor HojtsyHow is the removal of mixed_mode_sessions related? (From the end of the patch).
Comment #63
damiankloip CreditAttribution: damiankloip commentederrr hmmm , sorry. :)
The patch is good, apart from that straggler.
Comment #64
YesCT CreditAttribution: YesCT commented'string_int' is missing from the testDataTypes()
*if* this get a new patch, might as well take out this extra blank line introduced here.
Comment #65
damiankloip CreditAttribution: damiankloip commentedGood catch. That must have gone un-noticed for a while now :)
Comment #66
YesCT CreditAttribution: YesCT commentedusing @heyrocker's example re-written system.performance.yml:
without the patch, a diff of the saved active config:
with the patch, a diff the saved active config:
That's not what I expect. I thought this changed the config save format so that ints were not quoted. and I thought bools would be false or true.
Comment #67
alexpottThis patch does not fix what FAPI returns... if we want to do that we either have to cast the values in the form submit handler - or use the schema to somehow do this.
Comment #68
YesCT CreditAttribution: YesCT commentedis array a sequence? and noted with - instead of brackets?
like:
#1942110: Make breakpoint active config save format match the default yml file.
or.. #1933548: Book allowed_types settings repetitive and in under certain conditions can change unexpectedly
Maybe it should just be:
#1948284: [policy no patch] config save format and default yml file format and coding standards might have something to say about how to specify an empty array.
Comment #69
gddSorry but I'm still completely against this, because it means our core yaml files are going to be completely inconsistent, and it will be difficult to explain to developers how and why that is true. I agree with what everyone says about being able to save things in the format that it is submitted, but that can't be at the cost of a consistent understandable file. In some cases, if some of the data comes from FAPI and some doesn't, we may even end up with bools as FALSE or as '0' *in the same file* depending on how the data is being saved! This is just ridiculous and its going to confuse the hell out of people.
Sorry I just can't get behind this.
Comment #70
alexpottOk @heyrocker fair enough... I agree we have to stamp out the inconsistency...
So could we make the FAPI problem go away by adding the necessary casts to the submit handlers?
OR
using the schema to save config... which might not be a bad idea since then it would make it more likely that contrib will provide schema (a good thing I think). Actually this is the only way to go... because then drush / whatever else... can always save config correctly... and we get basic validation - like if the config is meant to contain an email address... make sure any value it has is a valid email address.
Comment #71
alexpottOk @heyrocker fair enough... I agree we have to stamp out the inconsistency...
So could we make the FAPI problem go away by adding the necessary casts to the submit handlers?
OR
using the schema to save config... which might not be a bad idea since then it would make it more likely that contrib will provide schema (a good thing I think). Actually this is the only way to go... because then drush / whatever else... can always save config correctly... and we get basic validation - like if the config is meant to contain an email address... make sure any value it has is a valid email address.
Comment #72
webchickOption A sounds like it depends on module developers doing special handling everywhere, which strikes me as "no." ;)
Option B would work, but makes config schemas a hard requirement, rather than a helper for multilingual config. I agree it's the only sensible way to solve this, though.
Comment #73
Anonymous (not verified) CreditAttribution: Anonymous commentedi'm not sure i understand the objections here.
variable_set() and variable_get() never mess with your data. also, variable_get() and variable_set() don't provide any help for you. if you put in a string and you should have put in an int, so be it.
so, this is a regression on a couple of fronts:
- if you know what you're doing, you can store types other than strings. gone
- whatever you put in, we give you the same thing back. gone
i don't think we need to require schema for casting. we should use it if it's there, so devs get rewarded for providing it.
and we should treat bogus type changes caused by cycling data through FAPI (or whatever reads then writes CMI data) as bugs.
Comment #74
webchickActually, option B wouldn't necessarily be a hard requirement. If the schema file's present, it could be used for casting; if it's not, just default to string. This would give module developers who knew what they were doing and cared about the nuance between FALSE, 0, NULL, and '0' the ability to do so, without pushing schemas as a requirement everywhere.
I forget also why we don't want schemas a requirement everywhere? What was the Cliff's Notes there?
Comment #75
webchickRe #73: The difference is that when datatypes are randomly swapping out from underneath you in D7, because you happened to submit a form that contained a value you may or may not have even changed, the only thing that knows that is the variable table. A site builder never had to be confronted with that fact; it happened totally transparently to them.
In Drupal 8, the difference between 1 and "1" is a diff when doing deployment changes that you don't remember ever doing. That's a big WTF.
I think what beejeebus is arguing though is "Yes, that IS a bug. So fix those on a one-off basis." Which is a fair stance to take, I guess, but given the requirement for module developers to spend time chasing down weird FAPI auto-casting-inspired bugs or just creating a schema file consistently, I'd prefer the latter. Seems much less frustrating.
Comment #76
webchickOh, the other option of course is fixing FAPI so it respects data types. :P I asked chx about this and he said:
"i wanted to release before 2015
the problem is the same as with any other generic fapi patch
there are MANY forms
he last time we did a wholesale conversion it made moshe think we will never release
since then surely did the community grow but also the core grown.
you need to tell fapi what the types and while most of it surely can be handled on a hook_elements level some of it will need to go to per element"
So um. Let's not try and tackle this here. ;)
Comment #77
catchI think it's OK to use the schema here. If schema's not defined then a UI might not be either?
Are there real cases where you'd not define a schema and expect everything else to work perfectly?
Comment #78
Gábor Hojtsy@catch: many people argued when config schemas were introduced that they should be kept as optional nice to haves that they don't need to think about at least in their contribs. Also, there is a very minimal set of people who actually worked on introducing the actual schema files (huge-huge props to @vijaycs85 especially). It would be great for them to get more real-life use so they are exposed to more than a couple people :D
Comment #79
gddYes, right now the schemas are completely optional from a functionality perspective. I believe the value they offer is identifying strings for translators, so even all the multilingual functionality will work without them. Gabor can correct me on this if I'm wrong.
I have a really icky feeling about making the schema required. They are not that easy to grok and write, and module developers have so many hurdles this release. I still continue to believe that casting everything to strings is our most sane option. (note i didnt say best)
Comment #80
webchickThis is all true. However, the advantage is that there are now multiple dozens of schema examples in core to copy/paste/modify from. I also found the documentation at http://drupal.org/node/1905070 quite excellent.
Comment #81
gddWhen you compare the learning curve of 'Everything is a string, the end' to the learning curve of 'Define this stuff over here in this nested schema file that is in yet another format you have to learn, and define the types you want to have in your main file. Don't forget to take into account the mapping for types like email, path, label, etc.'
I seriously and in my heart don't see what the horrible deal is with having everything be strings. It is simple and straightforward and it *functionally* hurts absolutely nothing. I'm obviously on the wrong side of this fight here so I'll drop it if I can't convince anyone else but man, this one seriously boggles my mind.
Comment #82
webchickWell, #73 lays out the cons. It's a DX trade-off either way. We're either forcing people to write schema files with obtuse syntax (which then has the benefit of providing the module with multilingual support) or we're forcing them to deal with frustrating and un-workaroundable typing issues that they don't deal with anywhere else in PHP/Drupal (except Form API, I guess...).
One idea that could be a possible compromise is, if a schema file exists, use it for data typing. If it doesn't, default to strings. Justin made that recommendation on IRC, but not sure if made it to the issue. Could that work?
Comment #83
webchickOr, I guess the other option is the last sentence of #73. We remove the string casting, and find places where things aren't typecast properly and cause diffs just due to saving a form, and fix them one by one.
I don't have a good sense of what that would look like for module developers. The hunks in BlockStorageUnitTest.php don't seem all that onerous, and is in fact how I would've written that code (and my YAML file, for that matter) anyway, had I not known that CMI was "special." But if a module developer would need to throw (bool) and (int) casts on every other line that would obviously be extremely annoying.
Comment #84
catchHmm I thought the compromise in #82 was the suggestion here... - use the schema if available, everything's a string if not.
Comment #85
webchickHm. This patch seems to remove the castValue() function in its entirety, so I don't think so. I think this patch is doing #83.
Comment #86
amateescu CreditAttribution: amateescu commentedYou are both right, that suggestion was made in IRC some while ago and @heyrocker posted it in the last paragraph of #53, but the current patch does *not* implement it.
Comment #87
tkoleary CreditAttribution: tkoleary commented@webchick: We're either forcing people to write schema files with obtuse syntax [ ] or we're forcing them to deal with frustrating and un-workaroundable typing issues
As part of my exploration of "big ideas" I have come across a very interesting new animal, the WYSIWYM (what you see is what you mean). It adds RDFa and microdata options to WYSIWYG. Here is the Tiny MCE demo: http://rdface.aksw.org/new/tinymce/examples/rdface.html
My purpose was more centered around content, not configuration but given your comment perhaps we should strive for a unified solution that improves both UX and DX.
I have already reached out to the lead of the project and asked if they had any plans to extend this to CK.
Comment #88
Gábor HojtsyYeah, the configuration system is using a very simple and easy to type YAML format which is not really RDFa or (attached) microdata compatible. Which is why we introduced the schema system in the first place to put on some level of metadata we needed about the structure and types from the outside. Expressing that data within the YML would break the simplicity which is/was its #1 selling point.
Comment #89
tkoleary CreditAttribution: tkoleary commented@webchick, @catch
This is also very interesting, if you are not already aware of it: http://www.easyrdf.org/
I think the WYSIWYM above depends on this
Comment #90
tkoleary CreditAttribution: tkoleary commented@Gábor Hojtsy
Ah. I see. Well nevertheless there are some tangential benefits that can be gleaned from continuing exploration of the following idea:
"how can we build interfaces for both users and developers that enable them to write fully machine-readable strings effortlessly using natural language."
Perhaps at a future point we will be able to expand that concept into this area.
Comment #91
YesCT CreditAttribution: YesCT commentedSo is next step here to implement the approach:
if a schema file exists, use it for data typing. If it doesn't, default to strings.
?
Comment #92
gddI still don't like that approach, as it is going to introduce a great deal of inconsistency in the YAML files, which is going to confuse new developers looking through these files for examples of how to format their own yaml. If we go this route I think we need to make schemas required, which I'm also against.
Comment #93
scor CreditAttribution: scor commented#90:
I don't think this is achievable given the timeframe for core, and it needs more research, prototypes and user feedback on the usability. Definitely an approach to explore as a contrib initiative à la spark.
Comment #94
tim.plunkettI just got burned by this while porting the fullcalendar module.
I very carefully trick FAPI and Views to get of the different options to be cast to the right type, but CMI turns them into strings, and the fullcalendar jQuery library errors out because it is getting '0' instead of false.
I think that's a fair compromise...
Comment #95
damiankloip CreditAttribution: damiankloip commentedThanks for helping confirm that keeping the string casting is not a good idea! I think this is going to be one of many... We can't just think of how this will be in core, but contrib too...
Comment #96
damiankloip CreditAttribution: damiankloip commentedRerolled, it's been a while.
Comment #97
alexpottSo I think that this sums up what I feel about changing everything to strings nicely...
http://books.google.co.uk/books?id=xColAAPGubgC&lpg=PA58&ots=q9ZBbeUR6x&...
Comment #99
damiankloip CreditAttribution: damiankloip commentedThis should fix a couple of them, There is something weird going on for the Image field test, doesn't happen in a normal env. Something to do with the file (or not in this case) being loaded...
Comment #100
dawehnershould better be public function ....
It seems to be that this test should maybe better a proper unit test?
config() should be better $this->container ...
Comment #102
dawehnerThe problem is the following.
This test creates an image field without a default image, so the default value is NULL. Previously it got casted to a string, so it seemed to be "0". This then got into entity_load, so it looked like array(0 => "0"). Now it is array(0 => NULL) and flipping does not work anymore.
This review was not really done properly.
Comment #103
damiankloip CreditAttribution: damiankloip commentedNice work on the fix, Lets do this.
Comment #104
catchTagging with D8 upgrade path - it doesn't block it, but we need to fix this before beta, do a beta-beta upgrade path (likely impossible), or move it to 9.x if it's not done by then.
Comment #105
damiankloip CreditAttribution: damiankloip commentedJust another reroll.
Comment #107
mtiftRerol
Comment #109
damiankloip CreditAttribution: damiankloip commentedLet's see how this gets on.
Comment #111
damiankloip CreditAttribution: damiankloip commentedLet's try this.
As a note, status should be fine in the long run (even with forms) as long as setStatus/enable/disable is called we will always have a bool there.
Comment #112
alexpottIf we are going to address this issue we need to do so before beta
Comment #113
alexpottxpost
Comment #115
tim.plunkettWorking through this, here's a couple fixes to start
Comment #118
tim.plunkettWe should follow this up with switching all of the default config values like status and locked to be actual bools, and weight to be an int, but if its not needed for the tests, we shouldn't explode this patch with them.
Comment #119
yched CreditAttribution: yched commentedI assume this relies on the assumption that all forms that save to a config entry somehow have an explicit step that casts the submitted values to their correct type, right ?
Looks like this will require some work for config entities that include 3rd party components (e.g field-type specific settings in field / instance config entities, field / filter / sort handlers in views, image effect settings in image styles...)
Comment #120
tim.plunkettYes. The point is that if you did have that step before, CMI would undo all of your work.
Comment #121
jbrown CreditAttribution: jbrown commentedSurely strings should always be quoted to avoid being misinterpreted as something else like boolean or octal, e.g. the zip code example.
Comment #122
tim.plunkettI don't disagree, but I don't know if that's in scope here. Strings without a space are fine without quotes, and this doesn't really affect that.
Straight reroll, no changes.
Comment #124
tim.plunketts/enable/install
Comment #125
Anonymous (not verified) CreditAttribution: Anonymous commentedthis looks RTBC to me.
Comment #126
jibranRTBC +1
Comment #127
vijaycs85Comment #128
webchickSince alex wrote a lot of this, and catch had proposed a compromise solution back a few dozen comments that was not implemented here, assigning to catch.
Comment #129
sunI'm not sure I understand why this is RTBC.
The discussed compromise back in #94 and before was:
However, the latest patch just removes the casting to strings entirely.
Unless I'm missing something, it does not appear to introduce any code that would (optionally) cast values according to data type definitions in the schema.
The stated compromise actually asks for EITHER casting to strings OR casting to schema-defined data types, depending on whether a schema is available.
Due to PHP's loose data typing, the compromise made sense to me. The current patch does not.
—
Lastly, you might want to remove the octal data type from the test, since your test results are misleading. You are not able to support octal numbers. YAML parses them correctly, but PHP interprets an octal number as an integer. When writing back to the file,
0775
becomes509
.assertIdentical()
reports success, because the data type and integer value is identical.Comment #130
tim.plunkettAdding schema based casting is a great follow-up! Feel free to open that.
The current system is broken, this removes the brokenness without solving any extra problems.
Comment #131
sunI think you're missing something big. The current
Config::castValue()
code path was added for very good technical reasons. IIRC, I originally disagreed with its addition, but was compelled after studying the (original) problems. As visible in the code, it is an extra (and expensive) operation that is executed. We wouldn't have introduced that operation if there wouldn't have been major problems. I'd have to study older issues and discuss with @heyrocker to recall why.Comment #132
alexpottAs far as I recall the current
Config::castValue()
is a hang up from when we were storing configuration in xml with no schema. Yaml and PHP serialization both support datatypes and I think it is reasonable to demand that whatever config storage system you use it returns data exactly the same as when it was provided. This function breaks that - I stand by my comment #97.Comment #133
catchI still think supporting data types via schema would be good, but for now manually casting when necessary is better than not having the option and having certain values be wrong all the time.
Having said that though, I don't see any code here to cast values from configuration form submissions - which is where this will go wrong, don't we need to update those here as well?
Comment #134
Anonymous (not verified) CreditAttribution: Anonymous commentedplease, no. this issue is about making the config system save what it's given, and send it back out.
fixing consumers of the config system is not for this issue. we don't have any fails, so we need to identify what if anything actually breaks, and fix the broken code.
when we fix that, we may also use schema to make it easier, but again, that is not for this issue.
Comment #135
catchHmm OK. I'm fine with doing that in a follow-up. Committed/pushed to 8.x - this will need a change notice, and we should open any follow-ups before closing this out.
Comment #136
Anonymous (not verified) CreditAttribution: Anonymous commentedok, i'll work on a change notice tomorrow when i'm sober.
as for the rest, i guess we need some tests? because if we broke anything, we can't prove it.
Comment #137
chx CreditAttribution: chx commentedI would like to see some thought put into whether assertIdentical is correct or we want to go key-by-key. AssertIdentical cares about array order and I think we don't. Or is this testing that the writing preserves the order as well?
Comment #138
webchickFirst one: #2105771: block.block.bartik.breadcrumbs and block.block.seven.breadcrumbs always show as need importing
Comment #139
Anonymous (not verified) CreditAttribution: Anonymous commentedsheesh. followed that issue.
chx pointed out we only really care about each value, patch coming that changes the test to do that, rather than comparing the whole lot (so we don't care about order).
Comment #140
andypostThis needs follow-up to convert all current .yml files to use proper types, for example 'Tags' vocabulary have all values as strings
Comment #140.0
andypostreordered
Comment #141
vijaycs85Create sub-task with files that need to be changed. Few things still not clear:
1. Route name/Machine names are in quotes on some files and other files not.
2. most of the info.ymls have true and other places we have TRUE
3. Version number in info are in quotes and some place no quote.
We have to make some standard around the above and update necessary files to the individual sub-tasks.
Comment #142
webchickHonestly, it feels like this was too premature. Everything failed super hard today when I went to demo this functionality in front of people at PNWDS. Did anyone test this patch with a real dev/live deployment scenario?
Comment #143
webchickFiled #2106171: Write tests for simple configuration deployment scenario about #142.
Comment #144
Gábor HojtsyI've been thinking what implications does this have for #1952394: Add configuration translation user interface module in core, which is another big producer of .yml files in config, which instead of updating files, it creates files based on the patterns of other files. I think this does not (yet?) apply there given the translation module works with all strings, no other types. If someone sees an issue there, we are welcome to adapt to the new ways - as always :)
Comment #145
chx CreditAttribution: chx commented#2106459: Core config has everything as string fixes most subtasks.
Comment #145.0
chx CreditAttribution: chx commentedAdding sub-task section.
Comment #145.1
vijaycs85Warning for sub-tasks
Comment #145.2
vijaycs85Updated issue summary.
Comment #146
krishnan.n CreditAttribution: krishnan.n commentedHi,
Will this (brute-force) script work:
Is something like the replacement below OK?
- items_per_page_label: 'Items per page'
- items_per_page_options: '5, 10, 20, 40, 60'
...
+ items_per_page_label: Items per page
+ items_per_page_options: 5, 10, 20, 40, 60
Thanks,
krishnan
Comment #147
krishnan.n CreditAttribution: krishnan.n commentedHi,
Changing status to 'needs review'.
Thanks,
krishnan
Comment #149
krishnan.n CreditAttribution: krishnan.n commentedConverting all '0' to false and '1' to true.
script to locate all yaml config files and then call the sed script above.
Notes:
1. need to run script in sequence, call sed script boolconvert.sed.
2. comments in #147 apply.
Comment #151
alexpottHi @krishnan.n thanks for your work here but this issue is "active" to get the change notice written (or an existing one updated). #2106459: Core config has everything as string already did most of the work that can be scripted. Unfortunately your scripts are a bit too greedy as well - we can not assume that all 0s and 1s should be true or false. Plus an empty string '' is meaningful and strings with a space should be surrounded by single quotes. If you want to help ensure that all the values are correct please help us work through the sub tasks in the issue summary and discover what still needs to be done. The important thing with default configuration files is that once the file is imported to active store when you diff the two files the only differences seen should be expected.
Comment #152
vijaycs85Thanks for working on this @krishnan.n, but this issue is to fix the type casting that we do on main process of configs. We have a list of follow-ups (https://drupal.org/node/1653026#sub-tasks) to fix individual modules. For your patch, you can either update them on subtask or raise a new issue (version 2 of #2106459: Core config has everything as string), so that we can leave this issue active for change notice.
Comment #153
krishnan.n CreditAttribution: krishnan.n commentedAlex, Vijay thanks for the inputs. Pl check if my fix 2105905 is OK?
Comment #153.0
krishnan.n CreditAttribution: krishnan.n commentedUpdated issue summary.
Comment #154
vijaycs85Comment #155
alexpottCreated followup #2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct
Comment #156
catchUnassigning me.
Comment #157
yanniboi CreditAttribution: yanniboi commentedHere is a draft for the change notice:
Summary
Currently in Drupal Core we're converting all config values to strings for storage because strings are common denominator for all possible storage engines. Since the storage process currently only deals with serialized PHP and YAML files, both of which support primitive data types, we are removing this forced type casting so that config values can be stored and retrieved in the same format without having to be converted into a string in the process.
Storage Example:
Before this change this would output:
After this change it outputs:
Examples:
Integers:
Before
weight: '0'
After
weight: 0
Booleans:
Before
status: '1'
After
status: true
Null:
Before
'visibility' => ''
After
'visibility' => NULL
Comment #158
jessebeach CreditAttribution: jessebeach commentedWhere are we moving the config data strings to? The last part of that phrase is a touch hard to parse, can you clarify please?
Comment #159
yanniboi CreditAttribution: yanniboi commentedYou are correct, I could hardly make sense of that myself..
I have changed it to read:
Comment #160
alexpott@yanniboi actually this paragraph is a bit misleading. This patch removed casting all values to strings so that we are now using YAML's ability to store scalar values that represent strings, integers, dates, and other atomic data types (see the spec). This means that a Config API stores values exactly as provided and will return them that way too. Perhaps the best way to illustrate this is like this:
Before this change this would output:
After this change it outputs:
Comment #161
yanniboi CreditAttribution: yanniboi commentedThanks @alexpott, that is very clear.
I have updated the change notice accordingly.
Comment #162
webchickThanks, yanniboi! Could you go ahead and add this as a change notice, so we can close this out?
I'd much prefer drafts to just go live as change record nodes immediately, rather than in comments, because even if they're not perfect, people can at least find them. :)
Comment #163
yanniboi CreditAttribution: yanniboi commented@webchick, that's all I needed to hear.
Here is the change record: [#2153567].
This issue can now serve as a meta for the sub-issues I think.
Comment #164
yanniboi CreditAttribution: yanniboi commentedComment #165
Gábor HojtsyIn the meantime I also copied and lightly edited yanniboi's suggestion. That change notice is at https://drupal.org/node/2153569. Eg. instead of saying "Drupal core currently does string casting", which is not true, we should say it used to :) Also the title of "All configuration values are stored as strings" of yanniboi's change notice was exactly what we are moving away from, so not very good to highlight the change IMHO. Sorry, but deleted his change notice and kept my copy, since we had two. Please edit that as needed.
Also since the META title is not anymore true here, adjusting to be more up to date.
Comment #166
xjmIt does not make much sense to postpone these changes to Drupal 9, and this does represent configuration schema changes. Bumping to critical accordingly after discussion with @webchick and @effulgentsia.
Comment #167
xjmHmm, actually, we should have a separate critical for the meta. The original change here has been resolved.
Comment #168
vijaycs85Comment #169
alexpott#2167623: Add test for all default configuration to ensure schema exists and is correct lets start testing default configuration and schema :)
Comment #170
yanniboi CreditAttribution: yanniboi commentedUnassigning myself.
Comment #171
vijaycs85All in-progress sub-tasks are closed as they all moved to #2167623: Add test for all default configuration to ensure schema exists and is correct. So setting this meta fixed.
Comment #172
Gábor HojtsyYay, thanks! Duplicate would be more appropriate.