[Dumper] Add support for XmlReader objets #18989

Taluu · Jun 7, 2016

Q	A
Branch?	"master"
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	~
License	MIT
Doc PR	~

Adding support for XmlReader objects. The thing is, with the caster as presented here, we may have noises (#text elements, whitespaces (significant or not) elements, ...) which may disturb the reading, but I am not sure if I should ignore those or not.

Moreover, there is only one class, but having different casters could do the trick I guess ? But then we have a lot of noise with empty results if none of the casters are satisfied (unless I missed something, allowing us to completely ignore the current object, in cases such as XmlReader::WHITESPACE and XmlReader::SIGNIFICANT_WHITESPACE).

On attributes, I can either not touch the current state, but then I can only have indexes for attributes and no names (<foo bar="baz"> would be rendered as array(0 => 'baz')), or I can modify the object (rendering <foo bar="baz"> as array('bar' => 'baz')) , but as @stof said, we should not touch it, unless it is not the actual object that is modified ? I do not have enough knowledge on the var dumper to determine that point...

Open for suggestions.

Add basic XmlReaderCaster
Add XmlReaderCaster in the default casters
Add some tests

stof · Jun 7, 2016

src/Symfony/Component/VarDumper/Caster/XmlReaderCaster.php

+class XmlReaderCaster
+{
+    private static $nodeTypes = array(
+            XmlReader::NONE => 'NONE',


wrong indentation

oops, fixed

& missing \ (XmlReader is not in Symfony\Component\VarDumper\Caster)

@Taluu I suggest you to start writing tests 😄

Taluu · Jun 7, 2016

Note ; I'm also not sure if I should dump all the xml, or just dump the current step (as it is done right now)

stof · Jun 7, 2016

src/Symfony/Component/VarDumper/Caster/XmlReaderCaster.php

+        if ($reader->hasAttributes) {
+            $infos['attributes'] = array();
+
+            while ($reader->moveToNextAttribute()) {


this changes the state of the reader, which is a no-go. The casters are not allowed to produce side effects.

Yes, I had a different implementation before, but I can't have the attributes name (only indexes). If that is enough, then I guess I could revert back.

But aren't the variables cloned or something like that when passed to the casters ? /cc @nicolas-grekas

This changes the state of the $reader object, is it possible to get the attributes doing any "move"?

Using something like this (Which is what I did before, but thought it was not really generous in information) :

<?php for ($i = 0; $c = $reader->attributeCount; $i < $c; ++$i) { $infos[Caster::PREFIX_VIRTUAL . 'attributes'][] = $reader->getAttributeNo($i); }

But then we are losing the attribute name...

stof · Jun 7, 2016

Tests should be added

nicolas-grekas · Jun 7, 2016

src/Symfony/Component/VarDumper/Caster/XmlReaderCaster.php

+    {
+        $infos = array(
+            'name' => $reader->name,
+            'local_name' => $reader->localName,


you should stick to the public property name (localName) so that one can learn by example how to use the object

fixed. I am then exposing 'localName' => $reader->localName, to stick with your other comments (and use the real property name)

Taluu · Jun 7, 2016

Hum, just thought of something else for the attributes, and getting the name. How about moving to the attribute node (effectively modifying the object) with moveToAttributeNo, but then reverting back to the current element with moveToElement ?

I could then get the name and the value of the attribute. But it does imply a little moving and going backwards though (what if there is an error in between ?).

What do you think of this @stof, @nicolas-grekas ? With a big fat warning if needed so ?

stof · Jun 8, 2016

With a big fat warning if needed so ?

This won't help much, as this warning would only be seen by developers of the component having to make it work, which means it should just work from the start (your big fat warning would be nothing more than a FIXME comment in practice).

nicolas-grekas · Jun 8, 2016

Maybe use readOuterXML or expand? I agree with stof, the warning would be useless.

Taluu · Jun 8, 2016

One of the point of using XMLReader is to be able to load a huge file, as DOM or SimpleXML needs to load everything in one go, then making the loading go berserk.... So using expand (which transforms the current node in DOM) or translating it as outerXML would I think defeat one of the point of using XMLReader.

And using that just to get the names of the attributes looks a bit overkill to me....

Taluu · Jun 8, 2016

After I gave it some thoughts, I think that using expand would not be really overkill (we don't want to dump ALL the nodes, just one node at a given time.. ?). Then exposing the expand as a virtual method could be useful too (and using this to determine the attributes too of course) ?

Taluu · Jun 11, 2016

ping @stof, @nicolas-grekas ; I am now managing the attributes with expand(), and I even exposed the dom as a cut stub (and virtual prefix).

I am not sure the dom should not be cut, but imo it kind of duplicates the main information of what is given by the main caster.

WDYT ? I'll add tests if that's good.

Output example : https://gist.github.com/Taluu/24c6c908fda299069c756a2be38f81dc

nicolas-grekas · Jun 16, 2016

src/Symfony/Component/VarDumper/Caster/XmlReaderCaster.php

+            'isEmptyElement' => $reader->isEmptyElement,
+            'nodeType' => new ConstStub(self::$nodeTypes[$reader->nodeType], $reader->nodeType),
+
+            Caster::PREFIX_VIRTUAL.'dom' => new CutStub($dom)


I'd suggest calling it expand, to help discoverability

renamed into expand

nicolas-grekas · Jun 16, 2016

some comments posted, time to add tests to me (and fix fabbot issue) :)

Taluu · Jun 16, 2016

Tests added /cc @nicolas-grekas, @stof

stof · Jun 16, 2016

src/Symfony/Component/VarDumper/Caster/XmlReaderCaster.php

+
+    public static function castXmlReader(\XmlReader $reader, array $a, Stub $stub, $isNested)
+    {
+        $dom = $reader->expand();


Wouldn't this convert the whole XML file into a DOM object in case the node being dumped is the root node ? This would make dumping incompatible with huge XML files

Yes, this was my point in an earlier comment, but I am not sure one would really dump such a huge xml with this... ?

if you are dumping the reader while debugging your code, you might not know which node is currently in the reader (otherwise you would not need to dump it)

True, but then I think I do not have any solutions for properly dumping attributes (or the expand() result actually)

I don't think dumping the expand result is needed

the difficulty comes from reading attributes, let's not dump them!

But then, I can dump them, but without their name (only their values). getAttributeNo does not consume the reader.

Taluu · Jun 17, 2016

I split the method (the special cases into their own methods) so that it is clearer. I am not a huge fan of the "NUMBER" constant, but I didn't want to calculate the whole array just to discard it anyway at the end...

Any ideas are welcome though.

nicolas-grekas · Jun 21, 2016

src/Symfony/Component/VarDumper/Tests/Caster/XmlReaderCasterTest.php

+
+    public function testNone()
+    {
+        $dump = <<<DUMP


<<<'DUMP'

Split main method into several sub methods when handling "special" cases such as ATTRIBUTE, NONE or filtered elements

nicolas-grekas · Jun 23, 2016

Continued in #18989
If you're OK @Taluu , I'll let you close this one.

Taluu · Jun 23, 2016

Closing then

This PR was merged into the 3.2-dev branch. Discussion ---------- [VarDumper] Add support for XmlReader objects | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18989 | License | MIT | Doc PR | - Commits ------- 3779ee4 [VarDumper] Add support for XmlReader objects

carsonbot added Status: Needs Review Feature labels Jun 7, 2016

stof reviewed Jun 7, 2016
View reviewed changes

nicolas-grekas reviewed Jun 7, 2016
View reviewed changes

Taluu force-pushed the xml-reader-caster branch 2 times, most recently from 27076b9 to df692ed Compare June 7, 2016 17:10

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Jun 8, 2016

nicolas-grekas reviewed Jun 16, 2016
View reviewed changes

Taluu force-pushed the xml-reader-caster branch 3 times, most recently from c16a577 to 4bfbcbd Compare June 16, 2016 14:53

Taluu force-pushed the xml-reader-caster branch from 4bfbcbd to 8d9b941 Compare June 16, 2016 14:59

stof reviewed Jun 16, 2016
View reviewed changes

javiereguiluz added the VarDumper label Jun 19, 2016

nicolas-grekas reviewed Jun 21, 2016
View reviewed changes

src/Symfony/Component/VarDumper/Tests/Caster/XmlReaderCasterTest.php

public function testNone()

{

$dump = <<<DUMP

Copy link

Member

nicolas-grekas Jun 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<<<'DUMP'

Taluu added 11 commits June 21, 2016 19:42

Add support for XmlReader objets

51cfb47

Add XmlReaderCaster in the default casters

5d83a71

Do not touch XmlReader state while reading attributes

71558e5

Ignore (partially) some node types

3250333

Add some more info on ATTRIBUTES elements

513e73b

Add unit tests for XmlReader Caster

5030ea5

Add parser properties virtual property

f442810

Add prefix and namespaceURI if declared

ec1764b

Add special case for NONE node types

f9f12fe

Use an Enum Stub for parser properties

9d87516

Move the parserProperties at the end of list

9049e45

Taluu force-pushed the xml-reader-caster branch from d8c36d8 to 822e78d Compare June 21, 2016 23:21

Refactor XmlReader caster

c5222fe

Split main method into several sub methods when handling "special" cases such as ATTRIBUTE, NONE or filtered elements

Taluu force-pushed the xml-reader-caster branch from 822e78d to c5222fe Compare June 21, 2016 23:37

nicolas-grekas mentioned this pull request Jun 23, 2016

[VarDumper] Add support for XmlReader objects #19151

Merged

Taluu closed this Jun 23, 2016

Taluu deleted the xml-reader-caster branch October 19, 2016 15:53

Search code, repositories, users, issues, pull requests...

Uh oh!

[Dumper] Add support for XmlReader objets #18989

[Dumper] Add support for XmlReader objets #18989

Uh oh!

Conversation

Taluu commented Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taluu commented Jun 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taluu Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Jun 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taluu commented Jun 7, 2016

Uh oh!

stof commented Jun 8, 2016

Uh oh!

nicolas-grekas commented Jun 8, 2016

Uh oh!

Taluu commented Jun 8, 2016

Uh oh!

Taluu commented Jun 8, 2016

Uh oh!

Taluu commented Jun 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jun 16, 2016

Uh oh!

Taluu commented Jun 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taluu commented Jun 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jun 23, 2016

Uh oh!

Taluu commented Jun 23, 2016

Uh oh!

Uh oh!

Taluu commented Jun 7, 2016 •

edited

Loading

Taluu Jun 7, 2016 •

edited

Loading