LibreDTE / libredte-lib-core

LibreDTE: Biblioteca PHP (Núcleo)
https://lib-core.docs.libredte.cl
GNU Affero General Public License v3.0
196 stars 156 forks source link

Problema en conversión XML a arreglo usando nodos con mismo nombre #62

Closed estebandelaf closed 5 months ago

estebandelaf commented 5 years ago

La clase \sasco\LibreDTE\XML permite cargar el contenido de un archivo XML y convertir este XML a un arreglo en PHP.

Cuando hay un nodo que se repite más de una vez se crea un arreglo con dichos nodos. Ejemplo, si tenemos el XML:

<Documento>
    <Detalle> blabla 1 </Detalle>
    <Detalle> blabla 2 </Detalle>
</Documento>

Se crea el arreglo PHP:

[
    'Documento' => [
        'Detalle' => [
            'bla bla 1',
            'bla bla 2',
        ],
    ],
]

O sea, se crea un arreglo con el nombre del nodo que se repite en el mismo nivel y se coloca cada nodo que repite su nombre como un elemento del arreglo.

Lo anterior funciona bien, de hecho es la base de LibreDTE y lleva años funcionando bien. Sin embargo, hay un caso que se detectó donde la conversión de XML a arreglo PHP falla:

Si el XML contiene un nodo que se repite en el mismo nivel (ej: nodo Detalle) y este a su vez tiene otro nodo que se repite en el mismo nivel (ej: nodo Subcantidad) entonces en este caso el resultado del arreglo PHP no es el esperado y falla.

Ejemplo que da problemas:

<Documento>
    <Detalle>
        <Subcantidad> código 1 </Subcantidad>
        <Subcantidad> código 2 </Subcantidad>
    </Detalle>
    <Detalle> 
        <Subcantidad> código 3 </Subcantidad>
        <Subcantidad> código 4 </Subcantidad>
     </Detalle>
</Documento>

Lo esperado es:

[
    'Documento' => [
        'Detalle' => [
            [
                'Subcantidad' => [
                    'código 1',
                    'código 2',
                ],
            ],
            [
                'Subcantidad' => [
                    'código 3',
                    'código 4',
                ],
            ],
        ],
    ],
]

Sin embargo el resultado es otro e incorrecto.

No es algo grave para la versión oficial en www.libredte.cl ya que oficialmente no se soporta el tag Subcantidad. Los usuarios que lo usan lo usan sólo una vez, sin repetir el tag Subcantidad dentro de un mismo detalle y en este caso (si no hay repetición) no se producen problemas.

El código a corregir está en el método XML::toArray() en https://github.com/LibreDTE/libredte-lib/blob/master/lib/XML.php#L265

Se deja abierta la issue para futura referencia y corrección cuando se decida hacer.

ffflabs commented 4 years ago

1. Si te fijas, salvo en el caso que no se pase un DOMElement y la instancia no tenga un documento cargado

https://github.com/LibreDTE/libredte-lib/blob/b75b92347223f422176b2f200c1599aa5d9df49f/lib/XML.php#L228-L230

En todos los otros casos el flujo de XML::toArray() muy resumido es

public function toArray(\DOMElement $dom = null, array &$array = null, $arregloNodos = false) {
   $array[$dom->tagName] =  $ContenidoDelDOM; // lo que sea que contenga
   return $array;
}

De manera que la salida siempre tiene la forma

 [$dom->tagName =>  $ContenidoDelDOM]

El array que recibe por referencia es mutado añadiéndosele una llave $dom->tagName. Por ejemplo para un DOMElement Subcantidad la salida de la función es

{ "Subcantidad":  "código 1"}

2. Cuando tienes elementos repetidos y creas a mano el índice numérico de los siblings para pasar por referencia al siguiente llamado de toArray

https://github.com/LibreDTE/libredte-lib/blob/b75b92347223f422176b2f200c1599aa5d9df49f/lib/XML.php#L271-L273

Digamos que estás parado en un nodo Detalle y estás recorriendo sus hijos que son las subcantidades. En cada bucle declaras

  $array['Detalle']['Subcantidad'][$siguiente] = [];

y luego cuando lo pasas al llamado recursivo

$this->toArray($child,$array['Detalle']['Subcantidad'][0], true);

se le asigna la tupla tag=>valor

$array['Detalle']['Subcantidad'][0] = ['Subcantidad' => "Código 1"];

Y por eso se duplica telescópicamente.

3. Cuando se indica a una recursión que es un arregloNodos, el único efecto que tiene es en

https://github.com/LibreDTE/libredte-lib/blob/b75b92347223f422176b2f200c1599aa5d9df49f/lib/XML.php#L256-L263

Me parece que esa condición está ahí para añadir un nodo directo al array y no a la llave proveniente de tagName, Pero justamente en el caso de tags duplicados-anidados nunca ocurrirá que los nodos gemelos sean uno solo.

Mi sugerencia

Aprovechando que el método no solamente muta el array sino que retorna su contenido, me parece que se podría minimizar los side effects si el valor de $array se va completando a partir del retorno de las llamadas recursivas y no a partir de mutaciones.

Usando el mismo ejemplo anterior. Estamos parados en una linea de detalle y estamos recorriendo subcantidades. Si hacemos

$childArray = []; 
$output=$this->toArray($child, $childArray, true);

ese $output sería

[ 'Subcantidad'  => 'Código 1']

De manera que luego de inicializar el array 'Subcantidad' en el padre 'Detalle' puedes pushear al arreglo sin tener que contar los nodos, pero más importante, puedes evitar la llave duplicada

// $array['Detalle']['Subcantidad'] = $array['Detalle']['Subcantidad'] ?? [];
$array[$dom->tagName][$child->tagName]=$array[$dom->tagName][$child->tagName]??[];

$array[$dom->tagName][$child->tagName][] = $output[$child->tagName]??$output;
// Esto es $array['Detalle']['Subcantidad'][] = [ 'Subcantidad'  => 'Código 1']['Subcantidad']
// Luego $array['Detalle']['Subcantidad'] = ['Código 1'];

(la coalescencia de output es por si entra a nodos gemelos === 1 y arregloNodos = true, que no devolvería el tagName repetido.

La cosa quedaría así:

 if ($nodos_gemelos==1) {
            // ...
   }
   // crear arreglo con nodos hijos que tienen el mismo nombre de tag
    else {
    $childArray = [];
    $output = $this->toArray($child, $childArray, true);
    $array[$dom->tagName][$child->tagName] = $array[$dom->tagName][$child->tagName]??[];
    $array[$dom->tagName][$child->tagName][] = $output[$child->tagName]??$output;
 }
estebandelaf commented 4 years ago

Gracias por el análisis.

Es algo que en algún momento futuro se debería corregir. El 2021 pretendo hacer algunos cambios al núcleo en cuanto a revisión de errores, crear pruebas y estilo de código. En ese momento podría ser buena idea retomar esta issue.

Sin embargo, al no ser algo que afecte a www.libredte.cl no tiene prioridad para ser corregido, al menos no de mi parte por ahora.

Lo que indicas queda como antecedente para una eventual corrección. Probablemente se pruebe directamente tu solución con algunos tests que se crearán y si los pasa se aplica el cambio. ¡gracias!