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
CommentFileSizeAuthor
#83 interdiff.txt2.59 KBjsbalsera
#83 2177799-83-unsigned-integer.patch16.08 KBjsbalsera
#78 interdiff.txt487 bytesjsbalsera
#78 2177799-78-unsigned-integer.patch18.39 KBjsbalsera
#76 2177799-72-76-interdiff.txt1.82 KBjsbalsera
#76 2177799-76-unsigned-integer.patch18.39 KBjsbalsera
#72 2177799-72-unsigned-integer.patch18.14 KBmauzeh
#71 2177799-71-unsigned-integer.patch18 KBmauzeh
#68 2177799-68-unsigned-integer.patch17.91 KBtstoeckler
#68 2177799-66-68-interdiff.txt829 byteststoeckler
#66 2177799-66-unsigned-integer.patch17.84 KBtstoeckler
#64 2177799-64-unsigned-integer.patch18.74 KBtstoeckler
#64 2177799-54-64-interdiff.txt1 KBtstoeckler
#54 2177799-54-unsigned-integer.patch17.39 KBtstoeckler
#54 2177799-52-54-interdiff.txt4.34 KBtstoeckler
#52 2177799-52-unsigned-integer.patch14 KBtstoeckler
#52 2177799-49-52-interdiff.txt980 byteststoeckler
#49 2177799-47-unsigned-integer.patch13.76 KBtstoeckler
#49 2177799-45-47-interdiff.txt753 byteststoeckler
#45 2177799-45-unsigned-integer.patch13.76 KBtstoeckler
#38 2177799-38-unsigned-integer.patch13.73 KBtstoeckler
#38 2177799-30-38-interdif.txt542 byteststoeckler
#31 2177799-27-29-interdiff.txt1.76 KBtstoeckler
#30 2177799-29-unsigned-integer.patch13.73 KBtstoeckler
#27 2177799-27-unsigned-integer.patch13.19 KBtstoeckler
#25 2177799-25-unsigned-integer.patch1.99 MBtstoeckler
#25 2177799-15-25-interdiff.txt17.01 KBtstoeckler
#16 2177799-14-unsigned-integer.patch14.27 KBtstoeckler
#16 2177799-14-15-interdiff.txt769 byteststoeckler
#14 2177799-14-unsigned-integer.patch14.27 KBtstoeckler
#14 2177799-12-14-interdiff.txt819 byteststoeckler
#12 2177799-8-unsigned-integer.patch14.28 KBtstoeckler
#8 2177799-8-unsigned-integer.patch14.28 KBtstoeckler
#8 2177799-5-8-interdiff.txt5.25 KBtstoeckler
#5 2177799-5-unsigned-integer.patch9.94 KBtstoeckler
#5 2177799-3-5-interdiff.txt2.08 KBtstoeckler
#3 2177799-3-unsigned-integer.patch9.22 KBtstoeckler
#3 2177799-1-3-interdiff.txt543 byteststoeckler
#1 2177799-1-unsigned-integer.patch9.23 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Status: Active » Needs review
FileSize
9.23 KB

Here we go.

Status: Needs review » Needs work

The last submitted patch, 1: 2177799-1-unsigned-integer.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
543 bytes
9.22 KB

Oops, the annotation was borked.

Status: Needs review » Needs work

The last submitted patch, 3: 2177799-3-unsigned-integer.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
9.94 KB

Wow, 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.

tstoeckler’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 5: 2177799-5-unsigned-integer.patch, failed testing.

tstoeckler’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.25 KB
14.28 KB

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

Berdir’s picture

Hah, I just had to look at those failed assertions and knew that would be fun ;)

Status: Needs review » Needs work

The last submitted patch, 8: 2177799-8-unsigned-integer.patch, failed testing.

The last submitted patch, 8: 2177799-8-unsigned-integer.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
14.28 KB

Trying again, just in case.

webflo’s picture

Only one nitpick. Looks good!

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/UnsignedIntegerItem.php
@@ -0,0 +1,60 @@
+ * Defines the 'unsigned_integer' entity field type.

This is an "field type" plugin not "entity field type".

tstoeckler’s picture

webflo’s picture

Status: Needs review » Needs work
Issue tags: +SprintWeekend, +D8MA

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

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
769 bytes
14.27 KB

Yup, thanks!

webflo’s picture

Status: Needs review » Reviewed & tested by the community

OK!

tstoeckler’s picture

Issue tags: +Entity Field API
sun’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, 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.

tstoeckler’s picture

Right, 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.

Berdir’s picture

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

tstoeckler’s picture

Ahh, 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.

tstoeckler’s picture

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

Berdir’s picture

schema() receives the FieldDefinition object as an argument, you can get it from that?

tstoeckler’s picture

FileSize
17.01 KB
1.99 MB

Thanks @Berdir for helping me out! That was quite stupid that I overlooked that...

Here we go, let's see how badly this fails.

Status: Needs review » Needs work

The last submitted patch, 25: 2177799-25-unsigned-integer.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
13.19 KB

Oops, forgot to merge 8.x. I should really know better by now... :-/

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerItem.php
    @@ -47,12 +47,25 @@ public function getPropertyDefinitions() {
    +
    +    $constraints[] = $constraint_manager->create('Range', array('min' => 0));
    +
    

    This is unconditional now, which I think isn't what you wanted?

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerItem.php
    @@ -47,12 +47,25 @@ public function getPropertyDefinitions() {
               'type' => 'int',
               'not null' => TRUE,
    +          'unsigned' => ((bool) $field_definition->getSetting('unsigned')) ?: FALSE,
             ),
    

    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?

amateescu’s picture

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

tstoeckler’s picture

FileSize
13.73 KB

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

tstoeckler’s picture

FileSize
1.76 KB

Forgot the interdiff

amateescu’s picture

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.

That's not entirely correct either :P That assumption does not apply to users, for example.

amateescu’s picture

Also, 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

Status: Needs review » Needs work

The last submitted patch, 30: 2177799-29-unsigned-integer.patch, failed testing.

The last submitted patch, 30: 2177799-29-unsigned-integer.patch, failed testing.

The last submitted patch, 27: 2177799-27-unsigned-integer.patch, failed testing.

The last submitted patch, 27: 2177799-27-unsigned-integer.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
542 bytes
13.73 KB

Oh, I will never learn how these annotations work...

This should install, at least.

sun’s picture

I 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).

Status: Needs review » Needs work

The last submitted patch, 38: 2177799-38-unsigned-integer.patch, failed testing.

tstoeckler’s picture

Re #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:

    $visitor->visit($this, $typed_data, $group, $propertyPath);

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 an IntegerItem) fails the is_numeric() check. It seems we shouldn't be passing the typed data object at all?!?

tstoeckler’s picture

Issue summary: View changes
tstoeckler’s picture

Title: Provide an 'unsigned_integer' typed data and field item » Allow IntegerItem's to be unsigned
Assigned: tstoeckler » Unassigned

Updated 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. :-/

tstoeckler’s picture

Issue summary: View changes
tstoeckler’s picture

Status: Needs work » Needs review
FileSize
13.76 KB

Oh, this needed a re-roll. Let's see if we still have the same fails.

Berdir’s picture

Some of those test fails look like they might be validating unsaved nodes without a nid, which would then not be a valid integer?

Status: Needs review » Needs work

The last submitted patch, 45: 2177799-45-unsigned-integer.patch, failed testing.

The last submitted patch, 45: 2177799-45-unsigned-integer.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
753 bytes
13.76 KB

Ok, 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. :-)

Status: Needs review » Needs work

The last submitted patch, 49: 2177799-47-unsigned-integer.patch, failed testing.

The last submitted patch, 49: 2177799-47-unsigned-integer.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
980 bytes
14 KB

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

Status: Needs review » Needs work

The last submitted patch, 52: 2177799-52-unsigned-integer.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.34 KB
17.39 KB
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
@@ -240,6 +241,8 @@ public static function baseFieldDefinitions($entity_type) {
+      ->setComputed(TRUE)

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

Berdir’s picture

This 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?!

Yeah, 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?

tstoeckler’s picture

Right, 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.

tstoeckler’s picture

Yay, 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.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yes, I think this is fine :)

tstoeckler’s picture

sun’s picture

Hm. 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?

tstoeckler’s picture

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

Berdir’s picture

Yeah, 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

We seem to be missing explicit test coverage.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
1 KB
18.74 KB

Here 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().

Berdir’s picture

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

tstoeckler’s picture

FileSize
17.84 KB

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

sun’s picture

"unsigned" and "signed" are very technical terms that describe storage constraints, not values.

+            'minMessage' => t('%name: The integer must be unsigned.', array('%name' => $this->getFieldDefinition()->getLabel())),

"%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."

tstoeckler’s picture

FileSize
829 bytes
17.91 KB

Alright, sure. Let's do it.

can i haz RTBC then pretty plz? :-)

Status: Needs review » Needs work

The last submitted patch, 68: 2177799-68-unsigned-integer.patch, failed testing.

mauzeh’s picture

Assigned: Unassigned » mauzeh

I'm working on getting the tests to pass.

mauzeh’s picture

Patch needed a reroll. Have not addressed the failed test yet. Will do that next.

mauzeh’s picture

Status: Needs work » Needs review
FileSize
18.14 KB

Changed label in test on rerolled patch.

sun’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
    @@ -63,9 +63,7 @@ public static function propertyDefinitions(FieldDefinitionInterface $field_defin
    +        ->setSetting('unsigned', TRUE);
    
    +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerItem.php
    @@ -17,6 +17,15 @@
    + *     "unsigned" = FALSE,
    

    The double-negation still melts my brain... :-/ — but ok...

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerItem.php
    @@ -17,6 +17,15 @@
    + *   settings = {
    ...
    + *   instance_settings = {
    

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

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerItem.php
    @@ -36,12 +45,40 @@ public static function propertyDefinitions(FieldDefinitionInterface $field_defin
    +          // Expose the 'unsigned' setting in the field item schema.
    +          'unsigned' => (bool) $field_definition->getSetting('unsigned'),
    

    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)

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.php
    @@ -235,6 +235,7 @@ function testEntityQuery() {
    +      ->sort('id')
    
    @@ -267,12 +268,14 @@ function testEntityQuery() {
    +      ->sort('revision_id')
    ...
    +      ->sort('revision_id')
    

    Not sure I understand why we suddenly need to sort the query results, but I assume this happened on purpose?

tstoeckler’s picture

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

jsbalsera’s picture

Assigned: mauzeh » jsbalsera

After talking with @tstoeckler I assign it to me and will work on it.

jsbalsera’s picture

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

field.integer.settings:
  type: sequence
  label: 'Integer settings'
  sequence:
    - type: string
      label: 'setting'

Should I change that for mapping and add the unsigned definition?

tstoeckler’s picture

Thanks @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:

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerItem.php
@@ -35,6 +26,26 @@ class IntegerItem extends NumericItemBase {
+  }
+  /**

There should be an empty line here.

Could you take care of that little thing, then we can knock this sucker out?

jsbalsera’s picture

Yeah, sorry about that!

Xano’s picture

Status: Needs review » Needs work

Looks like a few out of scope changes creeped in? Otherwise RTBC.

  1. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
    @@ -252,7 +253,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->addConstraint('CommentName', array());
    

    Out of scope.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.php
    @@ -235,6 +235,7 @@ function testEntityQuery() {
    +      ->sort('id')
    

    Out of scope?

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.php
    @@ -267,12 +268,14 @@ function testEntityQuery() {
    +      ->sort('revision_id')
    

    Out of scope?

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.php
    @@ -267,12 +268,14 @@ function testEntityQuery() {
    +      ->sort('revision_id')
    

    Out of scope?

jsbalsera’s picture

1. was introduced in #54, and it says:

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.

so probably is really out of scope.

About 2, 3 and 4 that code was discussed in #74:

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

So I better unassign this ticket and wait for @tstoeckler because I don't really know If I should make any changes :-)

jsbalsera’s picture

Assigned: jsbalsera » Unassigned
jsbalsera’s picture

Assigned: Unassigned » jsbalsera
jsbalsera’s picture

Status: Needs work » Needs review
FileSize
16.08 KB
2.59 KB

Getting rid of the "out of scope" parts

Xano’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! RTBC, under the condition that the tests pass.

The last submitted patch, 71: 2177799-71-unsigned-integer.patch, failed testing.

jsbalsera’s picture

Uhm, don't know why the bot tested #71, but last patch is green :D

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks great. Committed/pushed to 8.x, thanks!

  • Commit cf0549d on 8.x by catch:
    Issue #2177799 by tstoeckler, jsbalsera, mauzeh: Allow IntegerItem's to...
tstoeckler’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

alimac’s picture

Issue tags: -SprintWeekend +SprintWeekend2014

Minor tag cleanup - please ignore.