Project

General

Profile

Bug #90992

Updated by Volker Diels-Grabsch about 4 years ago

While developing an extension I noticed a misleading error message in the DataMapper. Hunting it down showed a clear programming error in @DataMapper::thawProperties()@. That method contains a big @switch@ over a string, where one case is a boolean instead of a string: 

 <pre> 
 switch ($propertyType) { 
    case 'integer': 
    ... 
    case is_subclass_of($propertyType, \DateTimeInterface::class): 
        $propertyValue = $this->mapDateTime( 
            ... 
        ); 
        break; 
    ... 
 } 
 </pre> 

 This error has been introduced in commit commit:aabe5f717. 

 Surprisingly, this code is not as broken as it seems. It does work, but only for subtle reasons: 

 It works because @switch@ uses @==@ instead of @===@ for comparison, and because @$propertyType == true@ holds for almost all strings @$propertyType@. Exceptions, of course, are the values @''@, @'0'@ and @null@. However, for those falsy values, the call to @is_subclass_of(...)@ returns @false@ as well, so the case comparison yields @true@ and the @mapDateTime(...)@ code is actually executed! Among others, this means that if the property type could not be identified, we get an error message regarding a failed DateTime conversion, instead of the proper error message generated by the @mapObjectToClassProperty(...)@ call in the @default@ case. 

 To complicate things, this bug has already been noticed in #89857, but has been "fixed" in commit:7ca1f49c9 by catching the case @$propertyType === null@ immediately before the @switch@. The cases of other falsy values such as @''@ and @'0'@ can still trigger the bug. 

 I propose to actually fix this programming error instead of adding more workarounds: 

 The @default@ case already contains @if@ statements for the more complex non-trivial cases, and originally even handled the DateTime case (until changed by commit commit:aabe5f717). So I propose to put the DateTime case back to its original position. 

Back