Fixes #6730 : close() xml reader on fromFile() and fromString()

[samsonasik]

Fixes #6730

[DASPRiD]

Shouldn't the same be done in fromString() then?

[samsonasik]

fromString() doesn't have open() that pointed to document, I think it doesn't need close then

[DASPRiD]

xml() calls also require close() calls, according to the documentation of it ;)

[samsonasik]

ok, I've added to fromString() too, thank you.

[samsonasik]

@Ocramius I think it's merge-able now ;)

[Ocramius]

@samsonasik is there no way to test this change?

[samsonasik]

@Ocramius $reader is protected property that instantiated via :

$this->reader = new XMLReader();

in fromFile() and fromString. Need public method like getReader to make it testable ?

[Ocramius]

@samsonasik no, there is no need to make it publicly accessible, but something may be triggering the bug here, no?

[samsonasik]

not sure, but fromFile and fromString always instantiate new class. so I think it is ok...

Warm regards,

Abdul Malik Ikhsan

Pada 22 Des 2014, pukul 10.48, Marco Pivetta [email protected] menulis:

@samsonasik no, there is no need to make it publicly accessible, but something may be triggering the bug here, no?

—
Reply to this email directly or view it on GitHub.

[Ocramius]

@samsonasik yes, but what is being fixed here? To give you a perspective of what should happen: if I remove this patch later on, will the change be noticed?

[samsonasik]

@Ocramius to check this, I think there is should be a function to check $reader->isValid() like this :

$xml = new XmlReader();
$xml->open('foo.xml'); // correct xml
$xml->setParserProperty(XMLReader::VALIDATE, true);

var_dump($xml->isValid()); // will return true
$xml->close();
var_dump($xml->isValid()); // will return false

how ?

[Ocramius]

Looks more like something that could cause misbehavior on multiple usages of the same reader instances: maybe try that and see if there is unexpected behavior?

[samsonasik]

@Ocramius ok, I will try that.

[samsonasik]

@Ocramius I've updated the code and added unit tests to test applying setParserProperty after using fromFile() or fromString() get php error warning. please let me know if there is something I missed ;)

[Ocramius]

I need to find a test case that doesn't need to access internal deps or check their state...

[Ocramius]

Pulling locally and attempting to fix the test

[Ocramius]

Rebased and merged, thanks @samsonasik!

master: bfc0eb6
develop: da67b48