sabre-io / xml

sabre/xml is an XML library that you may not hate.
http://sabre.io/xml/
BSD 3-Clause "New" or "Revised" License
516 stars 77 forks source link

Infinite loop in elementMap handler if not parsing inner elements #70

Closed judgej closed 8 years ago

judgej commented 8 years ago

This is my XML fragment:

        <despatchNote ID="383265">
          <detail ID="1" isbn="9781909633773" Qty="1" Status="OK" UMC="1.86"></detail>
          <detail ID="2" isbn="9781909633896" Qty="1" Status="--- Book not in Catalogue --" UMC="0"></detail>
        </despatchNote>

I want to turn each detail tag into a DespatchNoteLine object. The following goes into an infinite loop, creating DespatchNoteLine objects until the request runs out of memory.

        $reader->elementMap = [
            '{}detail' => function ($reader) {
                $attributes = $reader->parseAttributes();
                return new DespatchNoteLine($attributes);
            },
        ...

I get around this by parsing the inner elements (even though there are no inner elements):

        $reader->elementMap = [
            '{}detail' => function ($reader) {
                $attributes = $reader->parseAttributes();
                \Sabre\Xml\Element\Base::xmlDeserialize($reader); // <<<=== The fix.
                return new DespatchNoteLine($attributes);
            },
        ...

Is this expected behaviour? Should it be necessary to always explicitly parse or "consume" inner elements? My impression is that not parsing what's inside the detail elements would simply discard what's in there. But perhaps I'm looking at it wrong - the parser will parse everything and the mapping is a layer that sits on top of that.

Any thoughts?

evert commented 8 years ago

Yes you absolutely have to advance the reader, because the reader doesn't have a way to know what your intent is.

Thankfully it's pretty easy. Instead of \Sabre\Xml\Element\Base::xmlDeserialize($reader);, just call $reader->next();

Leaving this open as a documentation bug.

judgej commented 8 years ago

That works great - thanks.