Conversation
e7d125d to
d316f86
Compare
| $this->assertInstanceOf(EntityInterface::class, $newEntity); | ||
| $this->assertEquals('invalid', $newEntity->floaty); // which is weird but ok |
There was a problem hiding this comment.
It would be better if the entity reflected the state of the database after the save is complete. That would require us to reload the entity though.
There was a problem hiding this comment.
That would require us to reload the entity though.
Wouldn't it be enough if we set the value from the toDatabase() method of the used DB Type into the entity field back?
But I guess this only works for certain DB types as e.g. the DateTime type passes a formated string to the DB but we need a Date object inside the entity.
| $this->assertNull($newEntity->floaty); | ||
|
|
||
| // re-fetching the entity now results in 0, not null anymore as the DB value is not the same as the marshaled one | ||
| $newEntity = $table->get($newEntity->id); | ||
| $this->assertEquals(0, $newEntity->floaty); |
There was a problem hiding this comment.
This is strange. If the field was set to null, and the record was persisted, shouldn't the value be null?
There was a problem hiding this comment.
Thats exactly what I mean what is weird. We default to null in the marshaling process for unknown values, but the value being written to the DB goes through the toDatabase() method and therefore - in this case - always gets casted to a float unless its actually null or ''
Thats why null marshalled value will become 0 in this case in the DB, but we have other cases of this as well.
The base problem is still imho the fact the "lets default to null for unknown values" in marshalling.
d316f86 to
e52b695
Compare
This was caused by the discussion in #19153
TLDR: I think its weird, that we fallback to
nullfor unknown values on certain/most of our Database Types in the marshaling process. This PR tries to illustrate the weird behavior currently present in our marshaling process.The Problem
Under certain (maybe uncommon) circumstances, inconsistent behavior is present when trying to marshal data (from a request or "manually" built data passed down the entity).
Our current default behavior is, that we default to
nullfor unknown - not parsable values in the marshaling process which imho can cause weird edgecases as shown in the test added in this PR. (This is of course just one DB type of many, where similar behavior is present)As can be seen in the attached test I was only able to reproduce this when having no validation present for that field in the table validator, but maybe there are other cases also present which I haven't found yet.
Proposed solution
I'd argue we need to make DB type marshaling more "strict" and only allow passable/valid/correct data to be passed through and not default to
nullif its not valid.This would also of course enforce users to actually pass "null valid values" if they want null (usually its
nullor'')All other non-valid values being passed down to a field which don't fit (e.g. an array for a string field) would cause the mentioned exception which helps the user find the problem more easily and now wonder, why there is
nullin their column all of a sudden.