Conversation
Includes the older toArray conversion
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Infinite Recursion Risk ▹ view | |
 |
Complex Type Handling in Array Conversion ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| src/XmlNode.php | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| protected function _toArray(SimpleXMLElement|DOMNode|array $arr, Closure|null $func = null): array | ||
| { | ||
| if ($arr instanceof SimpleXMLElement) { | ||
| return $this->_toArray((array) $arr, $func); | ||
| } |
There was a problem hiding this comment.
Infinite Recursion Risk 
Tell me more
What is the issue?
The _toArray method contains potential infinite recursion when handling SimpleXMLElement instances.
Why this matters
If a SimpleXMLElement is passed, it will continuously cast to array and call itself, leading to a stack overflow.
Suggested change ∙ Feature Preview
Modify the method to handle SimpleXMLElement directly:
protected function _toArray(SimpleXMLElement|DOMNode|array $arr, Closure|null $func = null): array
{
if ($arr instanceof SimpleXMLElement) {
$arr = (array) $arr;
} elseif ($arr instanceof DOMNode) {
$arr = (array) simplexml_import_dom($arr);
}
$newArr = array();
if (!empty($arr)) {
foreach ($arr as $key => $value) {
$newArr[$key] = (
is_array($value)
|| ($value instanceof DOMNode)
|| ($value instanceof SimpleXMLElement)
? $this->_toArray($value, $func)
: (!empty($func) ? $func($value) : $value)
);
}
}
return $newArr;
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| protected function _toArray(SimpleXMLElement|DOMNode|array $arr, Closure|null $func = null): array | ||
| { | ||
| if ($arr instanceof SimpleXMLElement) { | ||
| return $this->_toArray((array) $arr, $func); | ||
| } | ||
|
|
||
| if ($arr instanceof DOMNode) { | ||
| return $this->_toArray((array) simplexml_import_dom($arr), $func); | ||
| } | ||
|
|
||
| $newArr = array(); |
There was a problem hiding this comment.
Complex Type Handling in Array Conversion 
Tell me more
What is the issue?
Method accepts multiple types (SimpleXMLElement|DOMNode|array) and has recursive type casting, making the flow hard to follow and maintain.
Why this matters
Violates SOLID's Single Responsibility Principle by handling multiple type conversions in one method. This complexity makes the code harder to test and more prone to bugs.
Suggested change ∙ Feature Preview
Split into specialized methods for each type:
protected function convertSimpleXmlToArray(SimpleXMLElement $element): array;
protected function convertDomNodeToArray(DOMNode $node): array;
protected function processArrayValues(array $input): array;Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
2 participants
Description by Korbit AI
What change is being made?
Add a new
toFullArray()method to theXmlNodeclass while also modifying the existingtoArray()method to handle bothDOMNodeandSimpleXMLElementinputs with an optional transformation function.Why are these changes being made?
The changes provide more flexible and comprehensive XML-to-array conversion options. The
toFullArray()captures all attributes and text nodes, whereastoArray()allows for customized processing of XML content, enabling easier manipulation of XML data in various applications. Adding comprehensive tests ensures the integrity and functionality of these methods.