282 discover all nested objects

[olegbaturin]

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	❌

#282
#286

1. Add tests for nested structures.
2. Update buildObjectsCache() to discover all nested objects only if required.
   4.1 Update dumpNestedInternal() to stop traverse deeper if object is not found in the cache. This prevents possible errors in the future.
   4.2 Fix dumpNestedInternal() to not stop dumping objects on the level 2*$depth.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.93%. Comparing base (9b140ac) to head (a7184a8).
Report is 19 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #287      +/-   ##
============================================
+ Coverage     76.15%   80.93%   +4.77%     
+ Complexity      586      525      -61     
============================================
  Files            48       45       -3     
  Lines          2034     1783     -251     
============================================
- Hits           1549     1443     -106     
+ Misses          485      340     -145     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[vjik]

Yet another test:

public function testAsJsonObjectsMapWithNestedObject(): void
{
	$nested = new stdClass();
	$nested->name = 'nested';
	$nestedId = spl_object_id($nested);

	$object = new stdClass();
	$object->name = 'root';
	$object->var = $nested;
	$objectId = spl_object_id($object);

	$this->assertSame(
		<<<JSON
		{
			"stdClass#$objectId": {
				"public \$name": "root",
				"public \$var": "stdClass#$nestedId (...)"
			}
		}
		JSON,
		Dumper::create($object)->asJsonObjectsMap(1, true)
	);
}

[olegbaturin]

Yet another test:

Both objects must be present in the expected result.

[vjik]

Both objects must be present in the expected result.

No. Depth is 1, so second object don't must be in result.

[olegbaturin]

Both objects must be present in the expected result.

No. Depth is 1, so second object don't must be in result.

It's a trap.

#282 (comment)

[olegbaturin]

Explanaition of buildObjectsCache()

In this PR, the buildObjectsCache() creates different $this->objects caches for asJsonObjectsMap() and asJson() methods.
For the asJsonObjectsMap() method with @vjik's improvements, it caches all possible objects from the $variable.

So this condition in the dumpNestedInternal() will never fire and can be removed indeed.
https://github.com/olegbaturin/yii3-debug/blob/7ca18453c342f027fa0c4bd7ab55c42b873ce3f0/src/Dumper.php#L174

Additional calls of the buildObjectsCache() from the dumpNestedInternal() also have no sense.

[olegbaturin]

Proposal for Callables dumping

Callables are dumped as strings and not as objects.
https://github.com/olegbaturin/yii3-debug/blob/7ca18453c342f027fa0c4bd7ab55c42b873ce3f0/src/Dumper.php#L159

But in the $this->objects cache Callable objects are still presented, and they are dumped without links to them from the parent properties.

"Yiisoft\\Definitions\\CallableDefinition#149": {
	"private $callable": "static fn (\\Yiisoft\\Aliases\\Aliases $aliases) => new \\Yiisoft\\Mailer\\MessageBodyTemplate(\n    $aliases->get($params['yiisoft\/mailer']['messageBodyTemplate']['viewPath']),\n)"
},
"Closure#147": "static fn (\\Yiisoft\\Aliases\\Aliases $aliases) => new \\Yiisoft\\Mailer\\MessageBodyTemplate(\n    $aliases->get($params['yiisoft\/mailer']['messageBodyTemplate']['viewPath']),\n)",

The proposal is to skip Callable objects in the buildObjectsCache() method.

"Yiisoft\\Definitions\\CallableDefinition#149": {
	"private $callable": "static fn (\\Yiisoft\\Aliases\\Aliases $aliases) => new \\Yiisoft\\Mailer\\MessageBodyTemplate(\n    $aliases->get($params['yiisoft\/mailer']['messageBodyTemplate']['viewPath']),\n)"
},

[vjik]

Proposal for Callables dumping

Can we make it in separate PR?

[samdark]

Is it good to go?

[vjik]

After discussion with @olegbaturin, we have suggestion.

1. Rename $depth parameter in asJsonObjectsMap() method to $arrayDepth.
2. $arrayDepth will be mean output depth for array object properties only.
3. And therefore, pass $depth+2 instead of $depth+1 in asJsonInternal().

@xepozz WDYT?

[xepozz]

Proposal for Callables dumping

Callables are dumped as strings and not as objects. https://github.com/olegbaturin/yii3-debug/blob/7ca18453c342f027fa0c4bd7ab55c42b873ce3f0/src/Dumper.php#L159

But in the $this->objects cache Callable objects are still presented, and they are dumped without links to them from the parent properties.

"Yiisoft\\Definitions\\CallableDefinition#149": {
	"private $callable": "static fn (\\Yiisoft\\Aliases\\Aliases $aliases) => new \\Yiisoft\\Mailer\\MessageBodyTemplate(\n    $aliases->get($params['yiisoft\/mailer']['messageBodyTemplate']['viewPath']),\n)"
},
"Closure#147": "static fn (\\Yiisoft\\Aliases\\Aliases $aliases) => new \\Yiisoft\\Mailer\\MessageBodyTemplate(\n    $aliases->get($params['yiisoft\/mailer']['messageBodyTemplate']['viewPath']),\n)",

The proposal is to skip Callable objects in the buildObjectsCache() method.

"Yiisoft\\Definitions\\CallableDefinition#149": {
	"private $callable": "static fn (\\Yiisoft\\Aliases\\Aliases $aliases) => new \\Yiisoft\\Mailer\\MessageBodyTemplate(\n    $aliases->get($params['yiisoft\/mailer']['messageBodyTemplate']['viewPath']),\n)"
},

Why?

[xepozz]

After discussion with @olegbaturin, we have suggestion.

1. Rename $depth parameter in asJsonObjectsMap() method to $arrayDepth.
2. $arrayDepth will be mean output depth for array object properties only.
3. And therefore, pass $depth+2 instead of $depth+1 in asJsonInternal().

@xepozz WDYT?

I don't mind if the name changed
Why $depth+2 instead of $depth+1?

[olegbaturin]

Why?

It's for your evaluation. If orphan lines for closures "Closure#147": "..." are required, just decline this proposal.

[vjik]

Variant 1. Consistent to to asJson() method

Level 0:

"array (1 item) [...]"

Level 1:

{
    "stdClass#2512": "stdClass#2512 (...)"
}

Level 2:

{
    "stdClass#2512": {
        "public $name": "nested1",
		"public $var": "array (1 item) [...]"
    }
}

Variant 2. Based on object level.

Level 0:

{
    "stdClass#2512": "stdClass#2512 (...)"
}

Level 1:

{
    "stdClass#2512": {
        "public $name": "nested1",
		"public $var": "array (2 items) [...]"
    }
}

Level 2:

{
    "stdClass#2512": {
        "public $name": "nested1",
		"public $var": [
			"test",
			"array (2 items) [...]"
		]
    }
}

Variant 3. Based on arrays.

Level 0:

{
    "stdClass#2512": {
        "public $name": "nested1",
		"public $var": "array (1 item) [...]"
    }
}

Level 1:

{
    "stdClass#2512": {
        "public $name": "nested1",
		"public $var": [
			"test",
			"array (2 items) [...]"
		]
    }
}

Level 2:

{
    "stdClass#2512": {
        "public $name": "nested1",
		"public $var": [
			"test",
			[
				"hello",
				"world"
			]
		]
    }
}

---

Does a result of asJsonObjectsMap() with collapsed objects make sense:

{
    "stdClass#2512": "stdClass#2512 (...)"
}

?

If no, we can use "Variant 3. Based on arrays." or "Variant 1. Consistent to to asJson() method", but limit minimum value of $depth to two.