Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Dumper] Add support for XmlReader objets#18989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| class XmlReaderCaster | ||
| { | ||
| private static $nodeTypes = array( | ||
| XmlReader::NONE => 'NONE', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
wrong indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oops, fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
& missing\ (XmlReader is not inSymfony\Component\VarDumper\Caster)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Taluu I suggest you to start writing tests 😄
Taluu commentedJun 7, 2016
Note ; I'm also not sure if I should dumpall the xml, or just dump the current step (as it is done right now) |
| if ($reader->hasAttributes) { | ||
| $infos['attributes'] = array(); | ||
| while ($reader->moveToNextAttribute()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this changes the state of the reader, which is a no-go. The casters are not allowed to produce side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This changes the state of the $reader object, is it possible to get the attributes doing any "move"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Using something like this (Which is what I did before, but thought it was not really generous in information) :
<?phpfor ($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 commentedJun 7, 2016
Tests should be added |
| { | ||
| $infos = array( | ||
| 'name' => $reader->name, | ||
| 'local_name' => $reader->localName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you should stick to the public property name (localName) so that one can learn by example how to use the object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed. I am then exposing'localName' => $reader->localName, to stick with your other comments (and use the real property name)
27076b9 todf692edCompareTaluu commentedJun 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 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 commentedJun 8, 2016
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 commentedJun 8, 2016
Maybe use |
Taluu commentedJun 8, 2016
One of the point of using And using that just to get the names of the attributes looks a bit overkill to me.... |
Taluu commentedJun 8, 2016
After I gave it some thoughts, I think that using |
Taluu commentedJun 11, 2016
ping@stof,@nicolas-grekas ; I am now managing the attributes with 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 |
| 'isEmptyElement' => $reader->isEmptyElement, | ||
| 'nodeType' => new ConstStub(self::$nodeTypes[$reader->nodeType], $reader->nodeType), | ||
| Caster::PREFIX_VIRTUAL.'dom' => new CutStub($dom) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd suggest calling it expand, to help discoverability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
renamed intoexpand
nicolas-grekas commentedJun 16, 2016
some comments posted, time to add tests to me (and fix fabbot issue) :) |
c16a577 to4bfbcbdCompareTaluu commentedJun 16, 2016
Tests added /cc@nicolas-grekas,@stof |
| public static function castXmlReader(\XmlReader $reader, array $a, Stub $stub, $isNested) | ||
| { | ||
| $dom = $reader->expand(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, this was my point in an earlier comment, but I am not sure one would really dump such a huge xml with this... ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
True, but then I think I do not have any solutions for properly dumping attributes (or theexpand() result actually)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think dumping theexpand result is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the difficulty comes from reading attributes, let's not dump them!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But then, I can dump them, but without their name (only their values).getAttributeNo does not consume the reader.
Taluu commentedJun 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. |
| public function testNone() | ||
| { | ||
| $dump = <<<DUMP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
<<<'DUMP'
Split main method into several sub methods when handling "special" casessuch as ATTRIBUTE, NONE or filtered elements
nicolas-grekas commentedJun 23, 2016
Taluu commentedJun 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
Uh oh!
There was an error while loading.Please reload this page.
Adding support for
XmlReaderobjects. The thing is, with the caster as presented here, we may have noises (#textelements, 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::WHITESPACEandXmlReader::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 asarray(0 => 'baz')), or I can modify the object (rendering<foo bar="baz">asarray('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.