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.
Updated: Comment #0
Problem/Motivation
in #1653026: [META] Use properly typed values in module configuration we removed the automatic casting of all config scalar values to strings.
Proposed resolution
Use typed config during Config::save()
ensure the values are cast to the correct type.
Remaining tasks
Review
User interface changes
N/a
API changes
Adds get($key)
method to Drupal\Core\Config\Schema\Sequence
so that schema wrapper objects for a particular key can be returned.
All changes to Drupal\Core\Config
are internal - if we need to open this up in the future we can change the protected methods added to public.
Comment | File | Size | Author |
---|---|---|---|
#123 | d8_typed_inheritdoc.patch | 2.4 KB | fago |
#119 | 2130811.119.patch | 43.45 KB | alexpott |
#119 | 115-119-interdiff.txt | 4.04 KB | alexpott |
#115 | 2130811.115.patch | 43.81 KB | alexpott |
#115 | 111-115-interdiff.txt | 5.07 KB | alexpott |
Comments
Comment #1
alexpottThe default config for system.performance.yml is
After changing some value on the form (admin/config/development/performance) and pressing save it is
Note that some of the key/values stored in this file are not editable on through this form.
Comment #2
alexpottOkay here is a patch that enforces the data type from a schema on save.
will update issue summary later reflect what this patch is doing and what's been discovered. As the patch shows the current schemas are far from complete and it's even discovered unnecessary configuration (system.maintenance should not have a enabled key!)
Comment #4
alexpottLet's see where this gets us
Comment #8
alexpottSo yay #2132551: Picture module uses config keys with a dot is causing us problems! Since this means the current schema for picture mapping entity the patch attached to this comment just comments out the schema.
Comment #9
alexpottAdding tests
Comment #12
alexpottThe field instance schema for the options lists are complete incorrect saying that they are a mapping and then declaring a sequence. In fact these field instances have no settings afaics so rather than saying anything about what the sequences contain I think we should just leave this empty.
Comment #14
alexpottExplaining some of the changes.
If we remove this try catch block then we will error on config files that have schema but do not conform to it. At the moment filter and views config entity do not conform.
This allows values to be null
Regions are strings the
-1
value is being used to denote no region.The changes are are because floats and decimals are not integers and the max and min have to be able to be nulls or empty strings. Casting a null or empty string to integer results in a max and min of 0 :D
This was moved to state in #2084339: The configuration value system.maintenance.enabled should be moved to the state system - I forgot to remove this value - if we enforce the schema to match what we have in config then this would have produced an error.
Comment #15
alexpottOk the field instance schema for option's fields are causing a problem - because they are just empty - ie. have no schema.
Comment #16
alexpottComment #17
alexpottSo the exceptions in the module install uninstall test in patches attached to #8, #9 and #12 are caused by having the content_translation module enabled before installing the options module. Content translation injects a setting into each and every field instance's settings - translation_sync - so there is no way the schema can reflect this. What can we do here? I'm not sure that we should allow dumping grounds in other module's config entities.
Also the statement in #15
is not true once the content_translation module is enabled.
Comment #18
xjmComment #19
damiankloip CreditAttribution: damiankloip commentedGreat, content_translation modules strikes again...
Comment #20
vijaycs85Thanks for the issue & patch @alexpott. Overall, it is great improvement & utilisation of config schemas. Here is some reviews comments:
Intentional?
Minor: Missing '.' at the end. Surely not a blocker :)
Comment #21
Wim Leers#20.2: yes, that's intentional, see #8.
Despite this being opened later, I have marked #2129399: Use configuration schema for configuration form type casting as a duplicate.
In #2105939: Make sure all YML files in Editor module has no type-casting to string + config system changes broke image uploading in editors, I introduced manual typecasting to ensure correct .yml files for
editor.module
(which is what spawned #2129399). I'm ecstatic to be able to delete that code here :)In this reroll, I fixed a bunch of nitpicks (including #20.3) and removed the code I introduced in #2105939. I don't know enough about the config system to do a solid review.
Comment #25
Wim LeersThis apparently makes the installer fail. But why? How is this different than what was there before?
Comment #26
alexpott@Wim Leers thanks for the fixing all the nits and removing the unnecessary code in editor! Yep this patch should mean that all modules need to do is declare the correct config schema and everything will just work (tm) :D
There was a slight error in the refactoring to Config::setSchemaWrapper()...
Interdiff was weird... here is the relevant bit:
Comment #27
yched CreditAttribution: yched commentedre #17 / translation_sync: opened #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition
Comment #28
alexpott26: 2130811.26.patch queued for re-testing.
Comment #30
xjmFail is:
Not sure if the last fail was the same or not, so triggering a retest just to be sure.
Comment #31
xjm26: 2130811.26.patch queued for re-testing.
Comment #33
xjmSame fail again, plus we picked up another. Fab. Both also failed when I ran the test on simplytest.me under minimal. The verbose result immediately preceding the config whatever list failed assertion shows "access denied" so that one at least should be easy to solve. :)
Comment #34
vijaycs85As per #8, we don't have picture schema anymore, so need to comment out picture test?
Comment #35
alexpottNow that #2132551: Picture module uses config keys with a dot has landed we can re-instate the picture test and not comment out the schema.
This was a reroll too so interdiff not possible so attaching diff of 34 and 35.
Comment #37
Gábor HojtsyRerolled. It was conflicting in edit module due to getFieldSettings() => getSettings() change. No change.
Comment #38
Gábor HojtsyReviewed the changes in the patch too. The schema changes are beautiful :) The only thing that stepped out is:
Should we open an issue for this or do this right away here?
Otherwise I'd say this look as good as it can :)
Comment #40
Gábor Hojtsy37: 2130811.37.patch queued for re-testing.
Comment #41
Gábor Hojtsy"Setup environment: failed to create checkout database." was the fail. No wonder it took so long to come back :)
Comment #43
vijaycs85Fixing test fails...
Comment #44
alexpottAnd improving the @todo :) mentioned by @Gabor
Comment #46
alexpottHo hum I really can not write English sentences! Fixed the @todo to be
Whilst reviewing the patch and the issue summary I realised that the methods added to Config were mostly public although we currently have no use case. This was because I originally planned to implement this functionality in ConfigFormBase.
I've updated the issue summary as well to reflect the API addition.
Comment #47
alexpottForgot to add an interdiff :(
Comment #49
sunThere's an inconsistency between getSchemaWrapper() + setSchemWrapper() and getTypedConfigManager() here; the latter lacks a setter method.
I don't know whether we have an architectural standard for this in D8, but if there is any point in having a separate setter method (tests? or what is it for?), then we should consistently apply it - even more so, within the scope of a single class.
If there is no technical reason for the separate setter, then I wonder why the getter for the schema wrapper doesn't also use the on-the-fly inline instantiation as in the getter for the typed config manager?
I don't have a preference, all I'm pointing out and asking for is consistency. :)
Can we use !isset() here?
empty() only makes sense if you expect a value of FALSE to have a meaning when compared to NULL.
Wherever services are conditionally injected, isset() should be sufficient (and faster).
Do we want to add Floats?
TypedData has a type for floats, and at least YAML supports floats, too.
Do we want to use the performance-optimized combined isset() || array_key_exists() pattern here?
Comment #50
Gábor HojtsyI'll take a crack at those :)
Comment #51
Gábor Hojtsy@sun:
1. Looked into this and discussed with @alexpott on IRC. Basically the schema wrapper depends on the config name *and* the whole data array. So if either the name or anything in the data array changes, the schema wrapper is not valid anymore. The patch had/has no invalidations sprinkled all around because this was introduced as protected internal functionality. So we can control it when we use it, and we only use it on save() which is also when we reset it. So the updated patch includes setting that value to NULL and integrates the set method into the get method. Logically doing the same thing either way. Added some more doc on the getter method about how the wrapper becomes stale quick.
2. Done.
3. Not only typeddata but also config schema has float types. I checked the system types and that was indeed the only one missed. Added coverage for casting and tests.
4. Done.
I also ran the image field display tests that failed above, but those did not fail for me locally. Will see if it fails again on testbot :)
Comment #52
Gábor HojtsyAll right, passed :) Any other concerns? :) AFAIS this should be ready to go in. And very timely :)
Comment #53
amateescu CreditAttribution: amateescu commentedThere is no \Drupal\Core\Config\TypedDataManager ?
Casting to boolean is not so simple, I'd recommend
filter_var($value, FILTER_VALIDATE_BOOLEAN)
.Missing 'string'.
Comment #54
Gábor HojtsyFixed 1 and 3. The TypedDataManager is indeed not in config, although the TypedConfigManager obviously is. Can you elaborate on 2? Provide a few examples as to why this is not good enough? Why is it good for ints or floats then?
Comment #55
amateescu CreditAttribution: amateescu commentedSee these two for example: http://stackoverflow.com/a/15075609/1499564 and http://matgargano.com/casting-a-string-to-boolean-in-php/
Comment #56
Gábor HojtsySounds to me casting 'yes' and 'on' to TRUE while casting 'off' and 'no' (and especially 'false') to FALSE is a feature request? The task in this issue is to ensure the type of the values matches the expected types by the schema, not to support magic values for them that cast to values in proper types.
Comment #57
penyaskito#55 I thought we wanted to make configuration idempotent. Allowing those can make tools like diff for comparing configs could behave weird because of the differences, while the underlying value could be the same.
Comment #58
amateescu CreditAttribution: amateescu commentedCool, carry on then.
Comment #59
Gábor Hojtsy@penyaskito: no the proposal of #55 was that if you do
And you have a schema defining 'key' as a boolean, it would save as false (whereas with the current patch it would save as true). So basically extend the scope of what PHP considers false/true values to more magic values. I think this is definitely out of scope for this issue and should be a followup.
Comment #60
Gábor Hojtsy@amateescu: any other concerns or do you think this is good to go then? :)
Comment #61
sunI agree with @Gábor. Intelligent filtering of magic (INI) string values into Booleans could lead to unexpected behavior. While configuration files may be edited by humans, humans need to respect the data type representations of the serialization format being used.
Comment #62
amateescu CreditAttribution: amateescu commentedSure, it's a one line change/improvement, it definitely needs to be a followup.
Comment #63
amateescu CreditAttribution: amateescu commentedNo other concerns :)
Comment #64
Wim LeersI agree with Gábor and sun too.
I reviewed the patch again, could find only one nitpick. But I don't feel I could RTBC this, because I don't know CMI well enough — I may very well be missing things. All I can say is that I strongly support this patch and that it feels like it's getting close.
Ideally, somebody with comprehensive CMI knowledge would review this patch to get it to RTBC.
The space between the function name and the parentheses shouldn't be there.
Comment #65
Gábor Hojtsy@WimLeers: thanks, fixed (by hand-editing, no interdiff attached).
@amateescu: It may be a one line change if we discount any documentation and test coverage. Also looks like a maybe innocent looking but controversial change to me.
Comment #66
sunThere's one data type that originally triggered the entire discussion on typed data for configuration values: Octals.
system.file.yml contains two chmod file permission masks:
I wondered whether this patch would help to resolve that original problem now (through a new DataType\Octal plugin):
But come to think of it, the schema-based data type conversions are happening way before the Yaml dumper is actually invoked, so even if we would attempt to properly convert an integer back to an octal representation, it would still be dumped as an integer, since the decoct() conversion would have to happen directly in the Yaml dumper, when the raw PHP value is serialized and written out into the Yaml document.
In turn, no improvement regarding Octals, bummer.
Nothing we can do about that I guess, so please ignore this part of the comment, just wanted to document considerations.
Why are the min/max settings of integer fields converted from integer to string?
Unless that isn't a mistake or temporary, that could use a comment.
The same applies to the min/max settings of decimal and float number fields (but of course, using float as data type instead of integer).
Comment #67
amateescu CreditAttribution: amateescu commented@Gábor Hojtsy, ok, ok, I stand corrected :)
Comment #68
Wim Leers#66.2: from #14.4:
Comment #69
vijaycs85@sun, regarding #66.1 - Please refer #2116701: Create Octal TypedData data type to support file mode and #2107689: Complete configuration schema for system module
Comment #70
sunHm. I see.
However, changing the data type to a string in order to avoid that issue completely defeats the point of having data types in the first place.
With the proposed patch, we're essentially declaring the configuration values to be arbitrary strings, whereas they may be numbers, but that is nowhere enforced, even though the consuming code exclusively interprets them as numeric values.
We have type casting now, but what we're missing is a conditional, type-specific handling of optional values, no?
In essence:
Expected result
(continuing Integer example)
Note:
~
is null in YAML.Comment #71
alexpottLet's address Sun's concerns about the null values here. It's be great if max and min could be integers imo.
Also lets inject the typed config manager in the constructor - discussed with @crell, @timplunkett and @sun on IRC and we agreed that we should
Comment #72
Wim LeersCan't we just use
-1
to mean "null" in this case?Comment #73
Gábor HojtsyRe: the integer empty question (#70/#71), I'm not sure we want to always cast an empty string as an int to a null. That sounds strange. If I set a typed value to integer and the user did not enter a value how is Drupal sure I wanted it to be saved as something different from zero? The "integer with a special don't care value" sounds like a "type" in itself to me. Even if we special case this in the casting system in config, the code will still need to take care of the default value maybe being null to not result in 0 in the output again. It is true \Drupal\number\Plugin\Field\FieldWidget\NumberWidget does not do anything special with the value and in itself defaults the value to NULL if none was provided. So I guess the question is maybe only if we want to have this magic apply to any integer value wholesale and if this is what people would expect from a general integer type (to not cast to integer all the time).
Re: the typed config manager injection: yeah. Will do.
Comment #74
Gábor HojtsyHere is all the fun you get for injection.
To constructor-inject Config, we'll need to obviously pass this in whenever it is instantiated. It is of course instantiated in ConfigFactory, which is a service, so it goes all the way through that to its service definition. But it is also instantiated from config.inc as well as ConfigImporter, which in turn has all its dependencies injected and instantiated from ConfigSync and various tests. I'm not even 100% sure I caught all the instances of where this needs to reach, but I think I caught most.
This is the type of change core committers usually ask to not do in a patch doing something else so it is still possible to review what the patch does otherwise.
Note I did not do anything about the integer problem yet, this probably in itself can do with a little round of review.
Comment #76
yched CreditAttribution: yched commentedWe can't, -1 (or any other actual number) is a totally valid value for "min" / "max"...
Well, in PHP anything can be NULL ? I don't think we want to duplicate all our types with a *_or_null variant ? It looks like we're facing the same issues than in a db layer. A db column is typed, but can still be "NULL".
Storing such a "not set" value when an integer value comes as '' sounds reasonable to me. If there is specific cases where "no input" should be interpreted as 0, then it's a job of the specific form / domain logic at hand ?
Comment #77
Gábor HojtsyOk this fails with:
Original
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "config.factory", path: "theme.registry -> config.factory -> theme_handler" in Symfony\Component\DependencyInjection\Container->get() (line 283 of /Users/gabor/Web/d8mi/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php).
Additional
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "theme_handler", path: "theme_handler -> config.factory". in Symfony\Component\DependencyInjection\Container->get() (line 283 of /Users/gabor/Web/d8mi/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php).
Which is puzzling for me. I don't see these circular dependencies based on the service definitions. We only change the factory dependencies and config typed is not dependent on anything with themes or vice versa. It has some shared dependencies with config factory, but that should not be an issue...
Comment #78
Wim LeersIn this particular case, you're right, but in general there are many places where negative values are nonsensical (e.g. "max upload size"), so there you can employ this solution.
It feels like in this case, we simply need a boolean
min_is_set
, and only when that's set totrue
, themin
value is used, otherwise it's ignored (and should probably be0
, though it doesn't matter).#74: wow.
Comment #79
Gábor Hojtsy@yched: well, SQL DDL has a [NULL | NOT NULL] constraint to attach to columns too for this reason. Anyway, let's assume ints may be null also. We need to apply this to floats as well then, since they have the same min/max/default setting where 0 and "none" are both meaningful values. So floats need the same code / test coverage.
That said this rolls back part of the original schema "fixes" however fixes the decimal and float field types to have min/max and default values typed as floats. Makes sense? :)
Note: still don't know how the circular service dependency arose, please help fix.
Comment #82
yched CreditAttribution: yched commented@Wim #78:
Ew :-( Split the info to an additional helper *_is_null property to be able to correctly interpret the way ConfigStorage stored the primary property ? Frankly, that sounds like a *very* kludgy and DX-painful abstraction leak...
In our case here, would mean NumberItem field type has to expose and manage two seemingly redundant *_is_set additional field settings, leading to: min, min_is_set, max, max_is_set - and keep them consistently set.
- This pollutes the DX of defining base fields as well, because you have to provide the right _is_null settings even if your definition is never going to be stored in YML
- This pollutes the DX of writing a field type. Field types are currently developped for base fields and configurable fields indifferently, but now you need to wonder about what will work for config storage.
- Frankly I'm scared of all the other places where we'd have to apply similar splitting.
This would boil down to "the moment there's any chance someone stores you in CMI, you cannot rely on PHP's way of handling NULLs". As I said, that's a very unfortunate and far-reaching abstraction leak IMO.
Comment #83
yched CreditAttribution: yched commented@Gabor #79
Right, and as you say, there's a reason for that :-). So maybe similarly we need a is_null boolean flag at the config DDL layer, i.e in config schema entries ?
Comment #84
Gábor HojtsyNow comes with installer service updates as well (based on suggestion from @alexpott) which was indeed the reason for the "circular dependencies" (not very helpful error, heh). Also fixed the property name references in config factory. This now installs for me at least :)
@yched: I'd rather not expand the scope of this issue even more. (Re: more schema attributes to do more specific typing).
Comment #85
Gábor HojtsySo @alexpott says we should preserve NULL on the string type as well (but not cast '' to NULL). Here you go. Any other concerns?
Comment #86
yched CreditAttribution: yched commented@Gabor: sure, the approach in the current patch, dealing with "empty number" internally in the storage layer, is good for me.
What I mean is, if we find more tricky cases that needs different logic, we should rather deal with them at the same place with additional DDL metadata, like SQL does, rather than forcing each upstream model logic to handle it themselves.
Comment #87
sunI think a new "nullable" definition property would be an alternative option, but like others, I'm scared of the declaration/definition overhead (since a lot of configuration values are optional), and I also wonder whether that definition wouldn't live in the wrong spot, since the functionality here is just about saving values and shouldn't involve any more sophisticated business logic.
If we would declare a nullable/emptiness flag, then I'd think we would actually have to invoke the type-casting algo in the setter method already (as opposed to the final/belated save operation). In my mind, a declarative flag in the definition would act like the 'required' flag; i.e., a field value validator (which should bail out during validation already).
But regardless of whether we will add an explicit definition property in the future, we'll need the proposed code logic here anyway.
As far as I can see, the proposed logic does not involve actual business logic, just a means of figuring out whether the input value represents a "cast-able" value — if not, then the typed value must be empty/null.
A "suspicious" gut feeling is understandable here.
I think it would be ok, since (1) all "arbitrary" values typically went through entity/form validation already, and (2) the scope is explicitly limited to values in configuration objects (a manageable scope).
The only possible conditions for all data types are null or empty string. The only exception is a data type of string, for which an empty string means an empty string, only null means null.
As a result, the check for a null value could actually be moved apart and simplified, because whenever a value is null, the saved value will also be null, regardless of data type.
Perhaps a larger question might be whether this kind of logic shouldn't be part of the data type plugins in the first place (as opposed to custom logic in a one-off function), but that's a topic for a follow-up. — I'm a bit surprised to see that our data type plugins are only able to handle values and do not actually care for the data type of the values being set, have no notion of empty/null values, and also no means for handling values of a different data type (which happens often in a loosely typed language like PHP).
Comment #91
sunComment #93
sunComment #95
alexpottPostponing on #2155785: image_field_instance_update() can not handle default images with a fid greater than 9 since #1973436: Provide config schema to field types storage for image module has landed recently and now we have bugs to fix. Yay!
Comment #96
fagoNot sure how far validation should go here, but given config schema is sort of typed data and typed data has built in the symfony validator for validation, I'm wondering whether it would make sense to make sure typed config is proper typed data (see #1905230: Improve the typed data API usage of configuration schema) and leverage that.
Comment #97
alexpott#2155785: image_field_instance_update() can not handle default images with a fid greater than 9 has landed
Comment #98
alexpott93: config.typed-values.93.patch queued for re-testing.
Comment #100
alexpottFixing the ConfigEntityTest
Comment #101
alexpottLets improve the test coverage
Comment #102
tim.plunkettI don't know TypedData very well, but this doesn't seem very extensible.
Couldn't these typed data classes have a
or whatnot? And then each can specify their cast.
Extra big indent here
Comment #104
alexpott@timplunkett actually @sun and I talked about adding a just such a method - it looks pretty nice.
Comment #105
Gábor HojtsyThis delegation certainly looks nicer than before. Good stuff!
If we want to bring this full circle though, we may want to resolve the schema defined type to a base type to ensure it gets the right base cast.
Eg. if I have
It would ideally come down to an integer cast, not a string cast.
It is true that all core non-primitive simple custom types are strings (text, label, path, etc), but that may not necessarily be true for contrib.
In fact since we have this, we need test coverage for the custom type anyway :) I think based on your code comment the above would cast the value to a string even though it resolves to an int, but based on the actual class resolving process, I think it may actually already resolve it to an integer?!
AFAIS needs more tests for those custom types then.
Comment #106
fagoI don't think there should be exception for '' - at least not at the typed data level. That would mean we interpret the boolean as string, but why would we? It's a boolean, so it should be interpreted as boolean.
Right now primitive classes are exclusive, i.e. implement just one primitive interface. This would make the uri class implement both.
I don't think this would be problematic, but I think it's safer code-path wise to keep not doing that. The number of primitives is low enough, such that one can easily make sure each primitive is handled properly. If that means handling URIs just as strings, that should be simple enough anyway.
Let's better clarify this casts to the correct PHP type. (not to the declared data type)
That should be fine, as the integer class casts using (int).
Yeah, contrib data types would just implement one of the primitive interfaces and would thus have to add the right casting method as well. So that should work as well?
Comment #107
alexpottComment #108
sunThe assumption of empty string → empty value is specific to the configuration system.
I absolutely +1 adding getCastedValue() methods on the data type plugins directly, but since those affect much more than just configuration, we'll need more accurate conditions there.
For example, both Integer and Float can implement:
That is because:
Since this is explicitly about type-casting, the following would not work:
Lastly, wouldn't the following method name be more accurate?
getTypedValue()
Or would that be confusing?
Slightly OT, but I still wonder whether this operation shouldn't be moved into the setValue() methods later on; e.g., like this:
Comment #109
fagoWe had that initially, but I don't think it makes sense as it's the job of validation to determine whether the value is correct, thus we shouldn't alter/massage the value beforehand. If we'd do the cast suddenly every float would become a valid integer, but it isn't. I think it's good to have the casting possibility, but it shouldn't be automatic.
Instead, we could name the method "castValue()" and just cast the set value + return it. I'm not sure which variant is more convenient here.
ad getTypedValue():
It's not clear to me what the "typed value" should be exactly, so yeah I think this is confusing. I'd prefer staying with casts as we all know what those do.
Comment #110
fagoImprovements in #107 look good to me.
While checking the interfaces, I noticed some dumb copy&paste mistakes :/ -> opened #2157787: Durations are not primitive.
Comment #111
alexpott@sun nice idea using is_numeric
@fago re Uri extending String - this is exactly what Email does.
Comment #112
fagoI'm not sure about the isNumeric(), it's not really the casted value any more.
>@fago re Uri extending String - this is exactly what Email does.
Yes, but Email does not implement two different primitive interfaces now, but URI would do so.
Comment #113
Gábor Hojtsy@fago: re #106:
I was talking of the *schema defined* types which cannot define their own PHP classes per say (using schema only at least, without writing PHP code :D) and @alexpott already added test coverage for that in #107, so looks good to me :)
So looks good to me, no outstanding issues for me.
Comment #114
vijaycs85Adding test case to check custom data type in sequence
Comment #115
alexpott@fago I think that the comment in the URI class implies it extends the primitive string type :)
That said if you truly think this is a bad idea I'm happy to add a getCastedValue() method to the
Drupal\Core\TypedData\Plugin\DataType\Uri
. I just can't work out why it is so bad to make it extend from String.Patch attached removes the use of is_numeric and introduces another function on the primitive interface
isNumericType()
@vijaycs85: I don't think we need another custom data type test - we have that covered. Using the parent stuff here just adds complexity and it is tested elsewhere in the same test file! An explicit sequence test is worth it though so thanks :).
Comment #116
Gábor Hojtsy@alexpott: I think there was value in testing that dynamic named types also properly resolve to their parent type, but that may be implicitly covered elsewhere.
Comment #117
fagoI don't think it's necessarily bad, but it's the way we've chosen to implement the primitives. Float does not extend from integer either, or Integer from string.
Comment #118
sunDo we really need to pollute the public API with a new method for this one-off use-case?
From my perspective, the typed data plugins are [too] complex enough already now, and I cannot really see a use-case beyond this config/schema system case for isNumericType()?
Perhaps, as a different approach: We appear to have a base String primitive that other types can extend from, but not a Number/Numeric primitive?
Or just a NumericInterface?
Thoughts?
Comment #119
alexpottHow about just checking the existing interfaces :)
Comment #120
Gábor HojtsyLooks great to me, resolving the above concerns. I think if we can make this even better later on, it is still a hugely valuable fix that will help us catch all kinds of config issues as time goes on :) Yay!
Comment #121
Dries CreditAttribution: Dries commentedCommitted to 8.x. HEAD. Data consistency FTW. :)
Comment #122
Gábor HojtsyAmazing, thanks! Wrote a change notice for this at https://drupal.org/node/2160639 :) Retroactively tagging Spark since I worked on this as part of my Spark dayjob.
Comment #123
fagoLooking at the typed data changes again, I've noted the docblocks use the wrong syntax - attached is a small follow-up patch for correcting these.
Comment #124
Gábor HojtsyLooks good. Moving to docs, so jhodgdon can commit it :)
Comment #125
Anonymous (not verified) CreditAttribution: Anonymous commentedi came across this code because i had to update #2098119: Replace config context system with baked-in locale support and single-event based overrides to work with it.
there may be implementation details i'm missing, but i thought we had agreement with 'what you put in to the configuration system is what you get out'? because this patch breaks that expectation completely.
can someone explain to me how that's a good idea? and yes, i know, http and forms and strings and typed data don't automagically match up and stuff, but i thought we decided that despite that, we were still going to make config give you back what was put in?
i guess i thought this patch was about validation? as such, what is it doing silently changing data?
Comment #126
Anonymous (not verified) CreditAttribution: Anonymous commentedwat wat wat from Config.php:
is that required because of some typed data magic? if so, can we please have a comment? if not, wtf?
Comment #127
webchickCommitted and pushed the doc follow-up in #123. Restoring priority. Bejeebus's questions still need answering, so leaving needs work.
Comment #128
sunUnfortunately, this caused a critical performance regression in the Drupal installer:
#2162013: Critical performance regression due to config schema + TypedData lookups in installer
Please do not roll back this patch. Let's fix the regression instead.
Comment #129
Anonymous (not verified) CreditAttribution: Anonymous commenteddiscussed this with alexpott in IRC.
apparently silently changing the data that is handed in is a feature, not a bug, so there's nothing else to explain about that.
i think this is completely the wrong choice, but i'm not going to fight about it, given that 'you don't get out what you put in' was intentional here.
so, only leaving this open for the NULL bit in #126.
Comment #130
alexpott@beejeebus so fine let's add a comment - we just preserve NULL values.
It's worth looking at the sample with a bit more context.
But lets do this is a separate issue rather that leaving a beta-blocking major open.
Comment #131
xjmAgreed. Please file separate followups for each of the above (including the docs fix).
Comment #132
xjmComment #134
Sutharsan CreditAttribution: Sutharsan commentedCreated a followup issue to remove the 'system.fast_404' schema which was not removed in this issue. See #2197873: Clean-up system.schema.yml to remove deprecated system.fast_404.