Conversation
![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 |
|---|---|---|
 |
Missing error handling for invalid XML in toFullArray ▹ view | |
 |
Inefficient Recursive Type Conversions ▹ 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.
| public function toFullArray(): array | ||
| { | ||
| $sxml = simplexml_import_dom($this->DOMNode()); | ||
| return [$sxml->getName() => $this->_toArray($sxml)]; | ||
| return [$sxml->getName() => $this->_toFullArray($sxml)]; | ||
| } |
There was a problem hiding this comment.
Missing error handling for invalid XML in toFullArray 
Tell me more
What is the issue?
The toFullArray() method doesn't handle the case where simplexml_import_dom() returns false, which can happen with invalid XML nodes.
Why this matters
If the XML node is invalid, the method will trigger a PHP error when trying to call getName() on a false value, causing unexpected crashes.
Suggested change ∙ Feature Preview
Add null check and throw an exception for invalid XML:
public function toFullArray(): array
{
$sxml = simplexml_import_dom($this->DOMNode());
if ($sxml === false) {
throw new XmlUtilException('Failed to convert DOM node to SimpleXML');
}
return [$sxml->getName() => $this->_toFullArray($sxml)];
}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); | ||
| } |
There was a problem hiding this comment.
Inefficient Recursive Type Conversions 
Tell me more
What is the issue?
Multiple type casting and conversions in recursive calls create unnecessary object instantiations and type conversions.
Why this matters
Each recursive iteration potentially creates new object instances and performs type conversions, leading to memory churn and slower execution, especially for deeply nested XML structures.
Suggested change ∙ Feature Preview
Normalize the input type once at the entry point and work with a consistent type throughout the recursion:
protected function _toArray($input, ?Closure $func = null): array
{
$arr = $this->normalizeToArray($input);
$result = [];
foreach ($arr as $key => $value) {
$result[$key] = is_array($value) ? $this->_toArray($value, $func) :
(!empty($func) ? $func($value) : $value);
}
return $result;
}
private function normalizeToArray($input)
{
if ($input instanceof SimpleXMLElement) return (array)$input;
if ($input instanceof DOMNode) return (array)simplexml_import_dom($input);
return $input;
}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?
Reintroduce a simplified conversion method
toArrayalongside the existingtoFullArraymethod and update related tests.Why are these changes being made?
The changes aim to enhance functionality by reintroducing the less detailed
toArraymethod while preserving the existingtoFullArrayfor comprehensive XML-to-array conversion. This allows for flexibility based on the required level of detail in various use cases, facilitating performance improvements where only a basic conversion is needed. The supplementary tests ensure compatibility and expected operation of both methods.