Fix XML parser in std.xml throws TagException if the attr value is put in apostrophes #4064

cosmo-kramer · 2016-03-07T18:52:40Z

This commit is an intended fix to https://issues.dlang.org/show_bug.cgi?id=4509

I'm a new contributer to Phobos,please tell me if there is something I should change.I have tested it on my local machine.

aG0aep6G · 2016-03-07T20:23:28Z

std/xml.d

+                string atr_val_put_chars="'\"";   //chars in which tags can be put into
+                reqs(s,atr_val_put_chars);
+                string val = decode(munch(s,"^'\""), DecodeMode.LOOSE);
+                reqs(s,atr_val_put_chars);


This behaves incorrectly for stuff like foo="bar 'baz' qux". You have to check which quote is used in the beginning, and stop when hitting it again.

aG0aep6G · 2016-03-07T20:34:43Z

I'm a new contributer to Phobos,please tell me if there is something I should change.I have tested it on my local machine.

When you make a pull request for a bugzilla issue, please leave a comment on the issue page with the pull request URL, and add the 'pull' keyword. You can also assign the issue to yourself.

Please add tests (in unittest blocks). When fixing a bugzilla issue, there's usually a test case which you can often simply copy. Here, also add a test for the mixed quotes thing I mentioned above.

aG0aep6G · 2016-03-08T14:07:45Z

I see you've added a commit. GitHub does not send out notifications for new commits, so generally you need to leave a comment if you want reviewers to notice that you did work on a pull request.

Also, you should add "fix issue 4509" to the commit message. This way the issue gets closed automatically when your pull request is accepted. And in the end, you should probably squash the commits into one. But we can polish up the code first, before dealing with these formalities.

aG0aep6G · 2016-03-08T14:08:02Z

std/xml.d

@@ -1,6 +1,6 @@
 // Written in the D programming language.

-/**
+/*


This is a mistake, isn't it?

dlang-bot · 2016-03-08T20:09:48Z

Fix	Bugzilla	Description
✓	4509	XML parser in std.xml throws TagException if the attr value is put in apostrophes.

cosmo-kramer · 2016-03-08T20:13:27Z

@aG0aep6G I have implemented your suggestions.
Thanks for your feedback.

aG0aep6G · 2016-03-08T21:24:19Z

std/xml.d

-                string val = decode(munch(s,"^\""), DecodeMode.LOOSE);
-                reqc(s,'"');
+                char quote = requireOneOf(s,"'\"");
+                char[] notQuote = ['^', quote];


You could still make this a char[2] to avoid an allocation. I don't know how important that is here, though. You would have to make it notQuote[] below then.

aG0aep6G · 2016-03-08T21:47:09Z

I was going to suggest you add asserts for the attribute values, but apparently no events are generated for the root element ... sigh ... That's a battle for another day, I suppose.

aG0aep6G · 2016-03-09T17:42:00Z

std/xml.d

@@ -2970,3 +2990,5 @@ private
        throw new XMLException(s);
    }
 }
+
+


Now there are three newlines at the end. One would be enough :)

aG0aep6G · 2016-03-09T17:44:13Z

I've updated the PR, sorry for few silly errors.

For some reason I did not get an email notification for that comment. I guess it's because the comment is on a commit rather than the pull request itself.

Code looks good to me now, aside from too many newlines at the end ;)

I think we should squash the commits into one, and format the commit message a bit nicer. If you're not super familiar with git, here's how I'd do that (not necessarily the quickest/best way to do it):

gitk to see how many commits we're dealing with: 4. Close the window.
git rebase -i HEAD~4 opens an editor with a list of commits and some instructions. Change "pick" to "squash" or "s" for all but the first commit. Save the file. Close the editor.
An editor opens again for the messages. Remove the messages of the first three commits. Change the remaining message to this:
```
std.xml: accept single quotes for attributes

Fixes issue 4509 - XML parser in std.xml throws TagException if the
attr value is put in apostrophes.
```
Save the file. Close the editor.
Open gitk again. Make sure everything is fine. Close the window.
Do a force push to GitHub to update the pull request: git push -f (or similar depending on how the local branch is configured).
Leave a comment on the pull request.

quickfur · 2016-03-09T20:03:42Z

std/xml.d

+                        xmlns="jabber:'client'" from='jid.pl' id="587a5767"
+                        xml:lang="en" version="1.0"></stream:stream>`;
+
+    DocumentParser parser = new DocumentParser(test_xml);


Should this also test that the parsed structure matches the expected structure? I.e., that the value of encoding is correctly parsed as UTF-8?

(And the xmlns in the stream:stream tag contains the entire value?)

Also, what about a tag like <p attr='a"b"c'/>? Does the parser handle that correctly?

Yes @quickfur it handles cases with " inside properly.I tested it.

Should this also test that the parsed structure matches the expected structure? I.e., that the value of encoding is correctly parsed as UTF-8?

Yeah, that should be tested. Unfortunately, std.xml doesn't seem to generate events for the root element, making this a bit more cumbersome than it should be.

@CoderAbhishek, if your motivation is still up for it, please add asserts for the values of the attributes. You can see how to do that in the unittest block that's below the one you added. Like there, you'll have to wrap things in a new root element. Also include a case like attr='a"b"c'.

Yes, once the asserts are added, we can merge this. Thanks!

Fixes issue 4509 - XML parser in std.xml throws TagException if the attr value is put in apostrophes.

cosmo-kramer · 2016-03-10T08:45:54Z

@aG0aep6G I've squashed the commits and updated PR.

Hackerpilot · 2016-03-18T22:44:28Z

@CoderAbhishek Please add the asserts suggested by the other reviewers. Once you do this one of us will merge the pull request.

cosmo-kramer · 2016-03-19T06:45:16Z

Sorry for delay,I'll do so soon.

On Sat, Mar 19, 2016 at 4:14 AM, Brian Schott notifications@github.com
wrote:

@CoderAbhishek https://github.com/coderabhishek Please add the asserts
suggested by the other reviewers. Once you do this one of us will merge the
pull request.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4064 (comment)

DmitryOlshansky · 2016-04-19T13:53:02Z

Sorry for delay,I'll do so soon.

Ping @CoderAbhishek

cosmo-kramer · 2016-04-19T14:27:10Z

Sorry I am a bit busy these days and I won't be able to complete it, at least not in a few weeks. Hope I have not disappointed you.

aG0aep6G · 2016-07-05T21:28:33Z

Resubmitted with asserts as #4567.

aG0aep6G · 2016-07-07T12:58:24Z

#4567 has been merged. This can be closed now.

quickfur · 2016-07-07T15:55:29Z

OK, closing.

aG0aep6G reviewed Mar 7, 2016
View reviewed changes

aG0aep6G reviewed Mar 8, 2016
View reviewed changes

std/xml.d

@@ -1,6 +1,6 @@

// Written in the D programming language.

/**

/*

Copy link

Contributor

aG0aep6G Mar 8, 2016

Choose a reason for hiding this comment

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

This is a mistake, isn't it?

aG0aep6G reviewed Mar 8, 2016
View reviewed changes

aG0aep6G reviewed Mar 9, 2016
View reviewed changes

quickfur reviewed Mar 9, 2016
View reviewed changes

std.xml: accept single quotes for attributes

63eae7e

Fixes issue 4509 - XML parser in std.xml throws TagException if the attr value is put in apostrophes.

cosmo-kramer force-pushed the master branch from c3da5d7 to 63eae7e Compare March 10, 2016 08:38

Hackerpilot added the Review:Needs Work label Mar 18, 2016

aG0aep6G mentioned this pull request Jul 5, 2016

Fix issue 4509 - XML parser in std.xml throws TagException if the attr value is put in apostrophes. #4567

Merged

quickfur closed this Jul 7, 2016

Uh oh!

Fix XML parser in std.xml throws TagException if the attr value is put in apostrophes #4064

Fix XML parser in std.xml throws TagException if the attr value is put in apostrophes #4064

Uh oh!

Conversation

cosmo-kramer commented Mar 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aG0aep6G commented Mar 7, 2016

Uh oh!

aG0aep6G commented Mar 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlang-bot commented Mar 8, 2016

Uh oh!

cosmo-kramer commented Mar 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aG0aep6G commented Mar 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aG0aep6G commented Mar 9, 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!

cosmo-kramer commented Mar 10, 2016

Uh oh!

Hackerpilot commented Mar 18, 2016

Uh oh!

cosmo-kramer commented Mar 19, 2016

Uh oh!

DmitryOlshansky commented Apr 19, 2016

Uh oh!

cosmo-kramer commented Apr 19, 2016

Uh oh!

aG0aep6G commented Jul 5, 2016

Uh oh!

aG0aep6G commented Jul 7, 2016

Uh oh!

quickfur commented Jul 7, 2016

Uh oh!

Uh oh!