Skip to content
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.

Sign up for GitHub

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

Add naive generic array implementation #244

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add naive generic array implementation #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8658c60
Add naive generic array implementation
danog Feb 19, 2025
74dc463
Fix
danog Mar 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions src/JsonMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,9 @@ public function map($json, $object)

$array = null;
$subtype = null;
if ($this->isArrayOfType($type)) {
if (null !== $subtype = $this->isArrayOfType($type)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's called $subtype, later down it is called $sub. I'd prefer a single name for a single thing.

//array
$array = array();
$subtype = substr($type, 0, -2);
} else if (substr($type, -1) == ']') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel we should keep these two codepaths that deal with the contents of arrays separated. But this is risky to change.

list($proptype, $subtype) = explode('[', substr($type, 0, -1));
if ($proptype == 'array') {
Expand Down Expand Up @@ -370,7 +369,7 @@ public function map($json, $object)
*/
protected function getFullNamespace($type, $strNs)
{
if ($type === null || $type === '' || $type[0] === '\\' || $strNs === '') {
if ($type === null || $type === '' || $type[0] === '\\' || $strNs === '' || str_starts_with($type, 'array<')) {
return $type;
}
list($first) = explode('[', $type, 2);
Expand Down Expand Up @@ -464,11 +463,11 @@ public function mapArray($json, $array, $class = null, $parent_key = '')
$class = $this->getMappedType($originalClass, $jvalue);
if ($class === null) {
$array[$key] = $jvalue;
} else if ($this->isArrayOfType($class)) {
} else if (null !== $sub = $this->isArrayOfType($class)) {
$array[$key] = $this->mapArray(
$jvalue,
array(),
substr($class, 0, -2)
$sub
);
} else if ($this->isFlatType(gettype($jvalue))) {
//use constructor parameter if we have a class
Expand Down Expand Up @@ -835,11 +834,17 @@ protected function isFlatType($type)
*
* @param string $strType type to be matched
*
* @return bool
* @return ?string
*/
protected function isArrayOfType($strType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename the method accordingly. I strongly believe a function with isSomething is supposed to return boolean for yes or no, and this has changed.

{
return substr($strType, -2) === '[]';
if (substr($strType, -2) === '[]') {
return substr($strType, 0, -2);
}
if (strpos($strType, 'array<') === 0 && $strType[strlen($strType)-1] === '>') {
return substr($strType, 6, strlen($strType)-7);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's this weird situation now that two of three cases where contents of arrays are being detected are covered here, the remaining one (examples are in the tests) with array[string] style is still outsourced in the logic.

Also, there are too many magic numbers happening here. I know we still support ancient PHP 7, so there is no way to utilize str_ends_with() etc. substr(..., -2) is borderline close to the threshold already, but standalone would pass as "cut last two chars off". The other case simply doesn't do it in an easier to understand way: What is the 6, what is the -7, why detect the last character in a different (array-access-like) way compared to the first case.

I see there is heavy complexity accumulating in this method now, and I know there's barely a way around it, but the responsible thing I ask for is to explain it better. And by explaining, I don't necessarily mean adding comments.

return null;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion tests/support/JsonMapperTest/Array.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class JsonMapperTest_Array
public $flArray;

/**
* @var string[]
* @var array<string>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the only string array with the square bracket style into something different does not sound like the best idea. I don't know if the float[] case above covers all relevant test cases, and I really would like to see a dedicated test case for this new type style, as the test acts as documentation what is possible. Hiding it behind some test map class code does not sound like a transparent way to handle new features.

*/
public $strArray;

Expand Down