IBX-11687: Fixed ContentTypeDomainMapper to not unset the default field value#755
IBX-11687: Fixed ContentTypeDomainMapper to not unset the default field value#755Sztig wants to merge 3 commits into
Conversation
…ruct when not set
…finitionFromUpdateStruct when not set
ibexa-workflow-automation-1
Bot
requested review from
Steveb-p,
ViniTou,
alongosz,
barw4,
bnowak,
ciastektk,
konradoboza,
mikadamczyk,
tbialcz and
wiewiurdp
and removed request for
a team
|
|
||
| self::assertNotNull($updatedFieldDefinition); | ||
| self::assertEquals($fieldDefinition->defaultValue, $updatedFieldDefinition->defaultValue); | ||
| self::assertSame(100, $updatedFieldDefinition->position); |
There was a problem hiding this comment.
It would be good to check position before updating the field definition.
| $fieldType->method('validateValidatorConfiguration')->willReturn([]); | ||
| $fieldType->method('validateFieldSettings')->willReturn([]); | ||
| $fieldType->method('isSearchable')->willReturn(true); | ||
| $fieldType->expects(self::once())->method('acceptValue')->with($expectedInput)->willReturn($expectedInput); |
There was a problem hiding this comment.
| $fieldType->expects(self::once())->method('acceptValue')->with($expectedInput)->willReturn($expectedInput); | |
| $fieldType | |
| ->expects(self::once()) | |
| ->method('acceptValue') | |
| ->with($expectedInput) | |
| ->willReturn($expectedInput); |
| $fieldType->method('validateFieldSettings')->willReturn([]); | ||
| $fieldType->method('isSearchable')->willReturn(true); | ||
| $fieldType->expects(self::once())->method('acceptValue')->with($expectedInput)->willReturn($expectedInput); | ||
| $fieldType->expects(self::once())->method('toPersistenceValue')->with($expectedInput)->willReturn($persistedValue); |
There was a problem hiding this comment.
| $fieldType->expects(self::once())->method('toPersistenceValue')->with($expectedInput)->willReturn($persistedValue); | |
| $fieldType | |
| ->expects(self::once()) | |
| ->method('toPersistenceValue') | |
| ->with($expectedInput) | |
| ->willReturn($persistedValue); |
|
|
||
| $spiFieldDefinition->fieldTypeConstraints->validators = $validatorConfiguration; | ||
| $spiFieldDefinition->fieldTypeConstraints->fieldSettings = $fieldSettings; | ||
| $defaultValue = $fieldDefinitionUpdateStruct->defaultValue ?? $fieldDefinition->defaultValue; |
There was a problem hiding this comment.
One thing that troubles me here is that the defaultValue in update struct could be set to null and it would be overriden by field definition's one because of ??
There was a problem hiding this comment.
If the intention is to have the field empty then the field value should be set to an empty string instead of null IMO. Entire struct here is set up in this way where null means that it was not updated.
There was a problem hiding this comment.
That is true for values sent from the Back Office - it's an empty string.
However, we also have an API. If one browses through contracts, one spots:
/**
* If set the default value for this field is changed to the given value.
*/
public mixed $defaultValue = null;one would think that setting it to null also sets the db column to null, but it's not the case, it skips the current value of FieldDefinitionCreateStruct. Maybe it would be enough to update description for this field in contracts? Open for discussion
There was a problem hiding this comment.
I can see your point, this would probably require some dedicated flag just for defaultValue I guess.
If clearing to null is a valid use case then it is kinda a separate API design problem and how this defaultValue in particular differs from the rest of the struct.
There was a problem hiding this comment.
yes, this is an API problem, however, imo in this PR we are fixing one issue with unitialized value that has a default but we are introducing a new one because the behavior is now changed for an initialized field with null value.
There was a problem hiding this comment.
we should populate defaultValue with current default at creation then?
There was a problem hiding this comment.
Could we handle this without ??? Right now, both "not provided" and "explicitly set to null" end up with the current default value, so API clients cannot intentionally clear an existing default value.
|
mikadamczyk
left a comment
mikadamczyk
left a comment
There was a problem hiding this comment.
Could we add an integration test for this API case? Something like existing field default value is set, then FieldDefinitionUpdateStruct->defaultValue is explicitly set to null? This would clearly document the intended contract and prevent regressions.
|
|
||
| $spiFieldDefinition->fieldTypeConstraints->validators = $validatorConfiguration; | ||
| $spiFieldDefinition->fieldTypeConstraints->fieldSettings = $fieldSettings; | ||
| $defaultValue = $fieldDefinitionUpdateStruct->defaultValue ?? $fieldDefinition->defaultValue; |
There was a problem hiding this comment.
Could we handle this without ??? Right now, both "not provided" and "explicitly set to null" end up with the current default value, so API clients cannot intentionally clear an existing default value.
7 participants
Description:
This is a fix to prevent
ContentTypeDomainMapperfrom unsetting the default field value when updating content type through API. This happens when the default value is not provided during the update.Added test coverage for this case as well.