Skip to content

Commit

Permalink
Fixes #5402: Debugger was not loading when there were closures, resou…
Browse files Browse the repository at this point in the history
…rces or PDO instances in the logged data
  • Loading branch information
samdark committed Oct 19, 2014
1 parent 9061874 commit 084d355
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 1 deletion.
1 change: 1 addition & 0 deletions extensions/debug/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Yii Framework 2 debug extension Change Log
2.0.1 under development
-----------------------

- Bug #5402: Debugger was not loading when there were closures, resources or PDO instances in the logged data (samdark)
- Enh #5600: Allow configuring debug panels in `yii\debug\Module::panels` as panel class name strings (qiangxue)


Expand Down
43 changes: 42 additions & 1 deletion extensions/debug/LogTarget.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,53 @@ public function export()
$data[$id] = $panel->save();
}
$data['summary'] = $summary;
file_put_contents($dataFile, serialize($data));
file_put_contents($dataFile, serialize($this->replaceUnserializable($data)));

$indexFile = "$path/index.data";
$this->updateIndexFile($indexFile, $summary);
}

/**
* Replacing everything that is not serializable with its text representation
*
* @param mixed $value
* @return mixed
*/
private function replaceUnserializable($value)
{
if (is_scalar($value) || $value === null) {
return $value;
}

if (is_array($value)) {
foreach ($value as &$row) {
$row = $this->replaceUnserializable($row);
}
return $value;
}

if ($value instanceof \Closure) {
return '\Closure';
}

if (is_resource($value)) {
return 'resource';
}

if ($value instanceof \PDO) {
return '\PDO';
}

$properties = (new \ReflectionObject($value))->getProperties();
foreach ($properties as &$property) {

This comment has been minimized.

Copy link
@qiangxue

qiangxue Oct 19, 2014

Member

This change is dangerous. If the object contains recursive object references (e.g parent refers to child, while child refers to parent), it will cause infinite loop. I suggest we only handle Closure, resource and nothing else.

This comment has been minimized.

Copy link
@samdark

samdark Oct 19, 2014

Author Member

Closure is entirely possible in an object property. I can introduce reference tracking and remove recursion.

This comment has been minimized.

Copy link
@qiangxue

qiangxue Oct 19, 2014

Member

My other concern is that this may take too much time. In which scenario will the debug data contain objects?

This comment has been minimized.

Copy link
@samdark

samdark Oct 19, 2014

Author Member

Any panel is able to write any data including objects. Currently, object will get into LogPanel when it will be logged by end user.

This comment has been minimized.

Copy link
@qiangxue

qiangxue Oct 19, 2014

Member

I think we should optimize this rather than blindly treating the data to be complex data in all cases. It is important that we do not spend too much time in debugger as it may cause unexpected side effect, such as timeout in ajax requests.

I suggest we introduce a helper method such as safeSerialize(). The method is only called when necessary, such as in LogPanel when a log message is not a string.

This comment has been minimized.

Copy link
@samdark

samdark Oct 19, 2014

Author Member

That should be done basically in any panel then. For example, AssetPanel while looking quite OK still can contain closures: https://github.com/yiisoft/yii2/blob/master/extensions/debug/panels/AssetPanel.php#L60

This comment has been minimized.

Copy link
@qiangxue

qiangxue Oct 19, 2014

Member

I know the issue in AssetPanel. That's one of the main places to be fixed.
In fact, most panels don't have this problem.

This comment has been minimized.

Copy link
@samdark

samdark Oct 19, 2014

Author Member

Since panels are extendable I'd prefer to handle it at framework level rather than on panel level.

This comment has been minimized.

Copy link
@qiangxue

qiangxue Oct 19, 2014

Member

As I said, it is important we don't spend too much time in saving debugger data because it lies in the critical path of every request (even if this is only used during development).
Since every panel has to write save(), it makes sense to explicitly call the safe serialize method only when necessary.

This comment has been minimized.

Copy link
@qiangxue

qiangxue Oct 19, 2014

Member

I just reviewed all panels. I think we only need to deal with AssetPanel. It's unlikely LogPanel will have the problem (unless you log with complex objects).

This comment has been minimized.

Copy link
@cebe

cebe Oct 19, 2014

Member

I just reviewed all panels. I think we only need to deal with AssetPanel. It's unlikely LogPanel will have the problem (unless you log with complex objects).

👍 logged objects must be somehow serializable anyway.

This comment has been minimized.

Copy link
@samdark

samdark Oct 19, 2014

Author Member

OK. Will adjust it then.

This comment has been minimized.

Copy link
@qiangxue

qiangxue Oct 19, 2014

Member

Thanks.

This comment has been minimized.

Copy link
@samdark

samdark Oct 19, 2014

Author Member
$property->setAccessible(true);
$propertyValue = $property->getValue($value);
$property->setValue($value, $this->replaceUnserializable($propertyValue));
}

return $value;
}

/**
* Updates index file with summary log data
*
Expand Down
1 change: 1 addition & 0 deletions framework/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Yii Framework 2 Change Log
2.0.1 under development
-----------------------

- Bug #5402: Debugger was not loading when there were closures, resources or PDO instances in the logged data (samdark)
- Bug #5584: `yii\rbac\DbRbacManager` should not delete items when deleting a rule on a database not supporting cascade update (mdmunir)
- Bug #5601: Simple conditions in Query::where() and ActiveQuery::where() did not allow `yii\db\Expression` to be used as the value (cebe, stevekr)
- Bug: Gii console command help information does not contain global options (qiangxue)
Expand Down

0 comments on commit 084d355

Please sign in to comment.