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 #42
Problem/Motivation
IntegerItem's currently have no way to denote that they are unsigned. For certain fields, however, it makes sense to provide validation that they are unsigned and to reflect this in the field's schema.
Proposed resolution
Add an 'unsigned' setting to IntegerItem.
Remaining tasks
Make tests pass.
User interface changes
None.
API changes
The following fields are now validated to be positive using the type data validation system and the database schema:
- IDs and revisision IDs of all entities
- The refresh field of aggregator feeds
- The filesize of files
Comment | File | Size | Author |
---|---|---|---|
#83 | interdiff.txt | 2.59 KB | jsbalsera |
#83 | 2177799-83-unsigned-integer.patch | 16.08 KB | jsbalsera |
#78 | interdiff.txt | 487 bytes | jsbalsera |
#78 | 2177799-78-unsigned-integer.patch | 18.39 KB | jsbalsera |
#76 | 2177799-72-76-interdiff.txt | 1.82 KB | jsbalsera |
Comments
Comment #1
tstoecklerHere we go.
Comment #3
tstoecklerOops, the annotation was borked.
Comment #5
tstoecklerWow, this one took me a long time to debug even though it seems so obvious now. I have become so used to using late static binding that I did not think about the implications:
Because we use static::$propertyDefinitions in IntegerItem having UnsignedIntegerItem extend IntegerItem they were sharing the same $propertyDefinitions and all hell broke lose. :-)
To me this means we should probably not be using LSB for $propertyDefinitions but that's a different issue. For now I changed UnsignedIntegerItem to not extend IntegerItem. It's such a small class anyway that that's a marginal change.
Also found that 'filesize' of the File entity should be unsigned as well.
Comment #6
tstoecklerComment #8
tstoecklerHere we go.
The tests were (not!!!) fun to debug... for a reason that completely eludes me entity query returns the results in a different order than before. The tests however were using assertIdentical() to verify the array correctness and, thus, the order mattered. To fix this I added sorts to the appropriate entity queries to enforce the behavior that was previously implicitly assumed.
Again, I don't really know *why* the order is now different, but we shouldn't be relying on that.
Comment #9
BerdirHah, I just had to look at those failed assertions and knew that would be fun ;)
Comment #12
tstoecklerTrying again, just in case.
Comment #13
webflo CreditAttribution: webflo commentedOnly one nitpick. Looks good!
This is an "field type" plugin not "entity field type".
Comment #14
tstoecklerComment #15
webflo CreditAttribution: webflo commentedI applied the last patch and grep-ed for existing integer FieldDefinition. We should convert the parent property for Terms to unsigned_integer. The other properties with data-type integer are fine.
Comment #16
tstoecklerYup, thanks!
Comment #17
webflo CreditAttribution: webflo commentedOK!
Comment #18
tstoecklerComment #19
sunSorry, why can't this just be a property/setting on the existing Integer type?
In fact, it looks like all you're really doing here is to automate the "min" range definition?
If we create a new data type for every possible incarnation of all possible permutations of primitive settings, then we'll end up in chaos.
Comment #20
tstoecklerRight, as far as I know, it's not a trivial thing to add a validation constraint based on a setting. I tried that originally but couldn't get it to work properly. Maybe Berdir / fago can help.
OTOH, I'm not really sure if this is chaos. An unsigned integer is a primitive data type in many programming languages; it's not some random permutation it's a very well-known thing.
Comment #21
BerdirIt should be possible to return such a constraint and also adapt the change based on the settings.
See EmailItem::getConstraints() as an example.
I do agree that doing this based on a setting is closer to how we define in also in the schema.
Comment #22
tstoecklerAhh, that's helpful thanks.
I just saw this todo in TypedDataManager::getConstraints()
* @todo: Having this as well as $definition->getConstraints() is confusing.
and thought that I somehow need to make the typed data manager return the constraint directly (which I did not manage to do). Putting it in the field item class makes sense and should be doable.
Will roll this into the integer class.
Comment #23
tstoecklerSo I was about to implement this, but I saw that typed data doesn't really have support for settings. So I removed the typed-data UnsignedInteger class completely and instead implemented the getConstraint() on the UnsignedIntegerItem.
So far, so good.
However, FieldItemInterface::getSchema() is a static method and, thus, has no access to $this->getSetting('unsigned') or whatever.
So what to do? It seems that having a schema that is based on a setting is currently not possible?! I looked through some example, and couldn't find anything, but certainly not all.
Comment #24
Berdirschema() receives the FieldDefinition object as an argument, you can get it from that?
Comment #25
tstoecklerThanks @Berdir for helping me out! That was quite stupid that I overlooked that...
Here we go, let's see how badly this fails.
Comment #27
tstoecklerOops, forgot to merge 8.x. I should really know better by now... :-/
Comment #28
BerdirThis is unconditional now, which I think isn't what you wanted?
Both this and the code in getConstriaints() could use a comment to explain what we're doing.
AFAIK, we should also set a default value for this setting in the annotation?
?: FALSE seems unnecessary if the first part is casted to bool anyway?
Comment #29
amateescu CreditAttribution: amateescu commentedIf we're updating all content entity ID fields, we might as well add a 'autoincrement' setting that changes the schema to 'serial' instead of 'int' :)
Comment #30
tstoecklerRe #28: Thanks for that. All very, very useful points! Attached patch fixes that.
Re #29: I like the idea, in theory. But not sure that's actually semantically correct. For instance in an entity's 'revision table' or 'data table' the ID is in fact not a serial field but a simple integer field. In #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities I'm currently checking whether a given field name is equal to $entity_type->getKey('id') to change the type to 'serial' dynamically.
Comment #31
tstoecklerForgot the interdiff
Comment #32
amateescu CreditAttribution: amateescu commentedThat's not entirely correct either :P That assumption does not apply to users, for example.
Comment #33
amateescu CreditAttribution: amateescu commentedAlso, the point of #2144327: Make all field types provide a schema() was to make field item classes the ones entirely responsible for their schema, to prevent exactly this kind of assumptions/trickery.
I guess this means we need either:
1) two more settings: autoincrement and serial
2) an autoincrement setting for the Integer item and a separate Serial item
Comment #38
tstoecklerOh, I will never learn how these annotations work...
This should install, at least.
Comment #39
sunI think the discussion on autoincrement/serial should be moved into a separate issue, but FWIW, as opposed to 'unsigned', which should just be a setting, the idea of a dedicated Serial (or AutoIncrement) item would at least make more sense, because such a data item class could/would/should have a special read-only behavior, since the value is expected to be returned from the storage engine only (whereas whether that makes sense is a different question).
Comment #41
tstoecklerRe #39: Yes, I agree. Let's handle that in a follow-up. I think that will be non-trivial on its own.
Regarding the patch here I need some help from someone more knowledgable with the typed-data validation system. I don't know why this patch exposes this behavior, but I've found something very strange in
\Drupal\Core\TypedData\Validation\PropertyContainerMedata
. Specifically in line 30:However in the visitor you see the
visit()
having$value
as the second argument, i.e. where we pass$typed_data
. This$value
then gets passed down into the Symfony validation system all the way down into (in this case) RangeValidator where the typed data object (in this case anIntegerItem
) fails theis_numeric()
check. It seems we shouldn't be passing the typed data object at all?!?Comment #42
tstoecklerComment #43
tstoecklerUpdated issue summary and title for the new approach. Still need help fixing the tests.
Unassigning for now as there's not much I can do currently. :-/
Comment #44
tstoecklerComment #45
tstoecklerOh, this needed a re-roll. Let's see if we still have the same fails.
Comment #46
BerdirSome of those test fails look like they might be validating unsaved nodes without a nid, which would then not be a valid integer?
Comment #49
tstoecklerOk, this was missing the getFieldSetting() -> getSetting() change. This should get us back to the previous fails.
@Berdir: Wow, that's pretty smart, I will investigate. Given your track record I have no doubt in my mind that that will solve the majority of the problems here. :-)
Comment #52
tstoecklerThis one should be better.
So I wasn't following EmailItem's example exactly, because that is using a 'ComplexData' and I thought, well, an integer can't be considered complex data, right? So that shouldn't be needed?!
Wrong. What was happening was that the Range constraint was trying to validate the entire IntegerItem (which is why #41 happened) but it should be validating the 'value' column of the IntegerItem.
I find that a little bit unintuitive, but on some strange level it does make sense.
Anyway, let's see how far this gets us.
Comment #54
tstoecklerThis was the problem. I don't know why I accidentally introduced this, but this reveals a (IMO) pretty fundamental weirdness in the typed data validation: Computed properties are not considered for validation. That is because ContentEntityBase::getIterator() chooses to not include computed properties and the typed data validation system uses a foreach. It seems this is a bug?!
In trying to fix this I also found that many definitions were using setConstraints() when they were in fact wanting addConstraint(). Some of those fixes are unrelated, so I guess we could split that off.
Comment #55
BerdirYeah, not just validation but also widgets/formatters and everything else that loops over the fields. I'd suggest to discuss with @fago if we want to include them by default and instead let the code that doesn't want them (like storage) ignore them specifically?
Comment #56
tstoecklerRight, the problem is that different use-cases have different needs. But in the validation case it's a pretty huge WTF IMO, that any constraints you add to a computed field are completely ignored. But, yeah, let's discuss that elsewhere.
Comment #57
tstoecklerYay, tests pass. Now this could use some reviews and maybe an RTBC :-)?!
To my mind this is commitable even with the slightly expanded scope of changing setConstraints() to addConstraint() where appropriate, or even to a setting (which then automatically adds the constraint). I originally did this because I thought this would fix tests. And in fact 2 of those places actually replace a Range constraint with min value 0 with the proper unsigned setting which is totally in scope for this issue. But there are 3 changes which are not strictly related to this patch. If people are adament about it, I would, of course, split those into a separate issue. But again, I personally think that is unnecessary in this case.
Comment #58
BerdirYes, I think this is fine :)
Comment #59
tstoecklerBtw I filed #2207193: Don't allow validation constraints for computed properties for #54 / #55 / #56
Comment #60
sunHm. Does the double-negation of
"unsigned: FALSE"
really make sense in this case?A signed integer should be the default, no?
I can see the train of thought that "unsigned" is a common construct in SQL schema declarations and essentially just taken over from there — but I'm not sure whether it makes sense to take that over into an abstracted data type property definition format?
Wouldn't it make more sense to have a default of
"signed: TRUE"
, and allow to specify"signed: FALSE"
where applicable?Comment #61
tstoecklerHmm... in general you're absolutely right that a positive is preferable to a double negative, nice catch! I hadn't thought of that.
In this case, however, I'm not sure whether an "unsigned integer" is not a fixed term in computer science, which would suggest an exception to this rule. I personally thought of
unsigned int
in C when writing this, so it seemed very natural to me (which is also why I made this a separate data type at first), but that might be just me.Maybe a native speaker (maybe one that has studied CS?) can chime in.
Comment #62
BerdirYeah, while @sun has a point about the double negative, I agree that in this context, using unsigned is the norm. It's also consistent with hook_schema(). Would be weird to have a signed setting that then maps to the unsigned schema key IMHO.
Comment #63
alexpottWe seem to be missing explicit test coverage.
Comment #64
tstoecklerHere we go, here's an assertion for the constraint validation in EntityValidationTest.
We can't really unit test the class because of t() and (more importantly) \Drupal::typedDataManager().
Comment #65
BerdirTest addition looks fine. Seeing that validation message again makes me wonder, though, should it say integer should be positive/larger than 0? A variable can be signed or unsigned but not sure if it makes sense for an actual value?
Comment #66
tstoecklerNeeded a re-roll. It was the first time I saw "instance_settings", but it seems as though "settings" is still correct for this. Still sort of strange as it is sort of duplicated with "min".
Have not addressed #65 yet, although I personally find it natural to say that "-3 is not an unsigned integer" and "3 is an unsigned integer". Not a native speaker though, and there's no real literal translation of unsigned in German that is also a negative so not really sure.
Comment #67
sun"unsigned" and "signed" are very technical terms that describe storage constraints, not values.
"%name: The value must be larger or equal to @min."
That should actually be the default validation error message for Range constraints. If there is a "max", then the error message should immediately inform about that, too:
"%name: The value must be between @min and @max."
Comment #68
tstoecklerAlright, sure. Let's do it.
can i haz RTBC then pretty plz? :-)
Comment #70
mauzeh CreditAttribution: mauzeh commentedI'm working on getting the tests to pass.
Comment #71
mauzeh CreditAttribution: mauzeh commentedPatch needed a reroll. Have not addressed the failed test yet. Will do that next.
Comment #72
mauzeh CreditAttribution: mauzeh commentedChanged label in test on rerolled patch.
Comment #73
sunThe double-negation still melts my brain... :-/ — but ok...
#2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition just moved the default settings into static methods, so this patch likely needs to be re-rolled for that?
Why do we need to cast to a Boolean?
Speaking of, if this setting is stored in configuration, then we're missing a config schema declaration for the new unsigned property? (which in turn might make this type-cast obsolete)
Not sure I understand why we suddenly need to sort the query results, but I assume this happened on purpose?
Comment #74
tstoecklerRe 2. yeah, that needs to be fixed.
Re 3. That was because when the value was in an annotation it read in as a string. At least I think that was the reason. Not quite sure anymore.
Re 4. At some point above those tests failed without those changes. If you look at the actual code you will see that the test is doing queries without a sort() but is then asserting that the order is correct. It passes currently because SQL's default sorting is reliable to some point, but it's very fragile. We can rip that out again if you feel strongly (I think the tests pass now without it), but I don't really see a reason for that personally.
Comment #75
jsbalseraAfter talking with @tstoeckler I assign it to me and will work on it.
Comment #76
jsbalseraFixing 2, changing annotations for static methods.
About 3, the prefix suffix just does this:
'#default_value' => $this->getSetting('prefix_suffix'),
(NumericFormatterBase.php:41)
So I simply removed the casting to bool. Anyway, now that we use the static functions instead of annotations that shouldn't be a problem anymore, right?
About the config schema now we have:
Should I change that for mapping and add the unsigned definition?
Comment #77
tstoecklerThanks @jsbalsera! Looks great.
I didn't really follow the getDefaultSettings() / getDefaultInstanceSettings() discussion, so I'm not 100% sure, but the separation *looks* right to me.
One super minor nitpick:
There should be an empty line here.
Could you take care of that little thing, then we can knock this sucker out?
Comment #78
jsbalseraYeah, sorry about that!
Comment #79
XanoLooks like a few out of scope changes creeped in? Otherwise RTBC.
Out of scope.
Out of scope?
Out of scope?
Out of scope?
Comment #80
jsbalsera1. was introduced in #54, and it says:
so probably is really out of scope.
About 2, 3 and 4 that code was discussed in #74:
So I better unassign this ticket and wait for @tstoeckler because I don't really know If I should make any changes :-)
Comment #81
jsbalseraComment #82
jsbalseraComment #83
jsbalseraGetting rid of the "out of scope" parts
Comment #84
XanoLooks good! RTBC, under the condition that the tests pass.
Comment #86
jsbalseraUhm, don't know why the bot tested #71, but last patch is green :D
Comment #87
catchLooks great. Committed/pushed to 8.x, thanks!
Comment #89
tstoecklerOpened #2235111: EntityQueryTest is fragile and #2235125: Use DataDefinition::addConstraint() instead of ::setConstraints() for the issues reported above.
Comment #91
alimac CreditAttribution: alimac commentedMinor tag cleanup - please ignore.