-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds support for strict flat type mapping #239
base: master
Are you sure you want to change the base?
Conversation
All non-namespaced classes in JsonMapperTest are made part of the PSR0 autoloading. Exception still made for the "Zoo" collection of classes.
We do not need to check for this version anymore
Was never verified using the non-existing second parameter of expectException()
The tests never check if they contain anything
Resolves: cweiske#232
Only handle objects as array when they implement both ArrayAccess and Traversable. BC break! ---- Originally, JsonMapper handled objects extending ArrayObject as arrays. Extending own collection classes from ArrayObject is not always feasible (issue cweiske#175, cweiske#175), so a way was sought to rely on interfaces only. Patch cweiske#197 (cweiske#197) changed the implementation to check for the ArrayAccess interface instead of ArrayObject. This unfortunately breaks objects-that-allow-array-access-but-are-not-traversable-arrays (issue cweiske#224, cweiske#224), for example when you allow array access to properties stored in some internal variable. The correct solution is to check that the object implements ArrayAcces *and* Traversable - then we can be sure the object is intended to be used with e.g. foreach(). Resolves: cweiske#224
In addition to "int|null", JsonMapper now also supports "?int" - a syntax that was added to PHP in versino 7.1.0 ("Nullable type syntactic sugar"). Resolves: cweiske#235
Simple types like strings in arrays that expect an object will throw an exception now when bStrictObjectTypeChecking is enabled. BC break! Resolves: cweiske#225
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I admit I am not really happy about getting another flag introduced because of the test effort involved.
That being said, this ship sailed long ago, as this library is now full of flags anyways. But still: This flag needs tests! And documentation.
case 'string': return $jType === 'string'; | ||
case 'bool': return $jType === 'boolean'; | ||
case 'int': return $jType === 'integer'; | ||
case 'float': return $jType === 'double' || $jType === 'float' || $jType == 'integer'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gettype()
does not return "float"
according to php.net
case 'bool': return $jType === 'boolean'; | ||
case 'int': return $jType === 'integer'; | ||
case 'float': return $jType === 'double' || $jType === 'float' || $jType == 'integer'; | ||
case 'null': return $jType === 'null' || $jType === 'NULL'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gettype()
only returns "NULL"
* type as the given json type | ||
* | ||
* @param $classType | ||
* @param $jType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As much as I personally don't like PHPDoc anymore (preferring explicit types), the minimum here would be giving the types for the parameters.
throw new JsonMapper_Exception( | ||
'JSON property "' . $key . '" in class "' | ||
. $strClassName . '" is of type ' . gettype($jvalue) . ' and' | ||
. ' cannot be converted to ' . $type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't about "cannot", but "will not" or "should not", because the code that literally does it is on line 269/284 below.
* | ||
* @var bool | ||
*/ | ||
public $bStrictFlatTypes = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, another flag... which effectively requires doubling all test cases except the ones that are obviously failing their test case long before this strict flat type phase would be reached.
@esdraelon Will you provide unit tests for the new flag? |
Adds support for strict flat type mapping via $bStrictFlatTypes