Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further explain xml syntax change for 3.6.0 #725

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

mstovenour
Copy link
Contributor

No description provided.

Copy link
Member

@wdoekes wdoekes left a comment

Choose a reason for hiding this comment

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

Thanks for the PR :)

@@ -125,6 +125,9 @@ The following example is used to:
</action>
</recv>

.. note::
Release 3.6.0 added XML syntax checks and the characters & and < are
not valid in XML files. Replace those with &amp; &lt; in strings.
Copy link
Member

Choose a reason for hiding this comment

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

I would say:

  Release 3.6.0 added rudimentary XML syntax checks. Now the &
  and < characters must be escaped even inside attribute values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we explain how to escape them? Again I expect a lot of people to trip over this and many of them are not programmers and may have only ever used XML with SIPp. e.g. my entire SVT test organization.

Release 3.6.0 added rudimentary XML syntax checks. Now the &
and < characters must be escaped (& <) even inside attribute values.

Copy link
Member

Choose a reason for hiding this comment

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

We can. How about:

Release 3.6.0 added rudimentary XML syntax checks. Now the & and <
characters must be escaped as &amp; and &lt; even inside attribute values.


.. note::
Release 3.6.0 added XML syntax checks and the characters & and < are
not valid in XML files. Replace those with &amp; &lt; in strings.
Copy link
Member

Choose a reason for hiding this comment

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

same

src/scenario.cpp Outdated
@@ -1026,7 +1026,7 @@ scenario::scenario(char * filename, int deflt)
/* Close scenario element */
xp_close_element();
if (xp_is_invalid()) {
ERROR("Invalid XML in scenario");
ERROR("Invalid XML in scenario. Check for & and < in strings; replace with &amp; &lt;");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe:

Invalid XML in scenario. See: https://github.com/SIPp/sipp/issues/414

Because there could certainly be other reasons than unescaped lt and amp.

Copy link
Member

Choose a reason for hiding this comment

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

Here's a commit/branch that adds line numbers in some cases:

a3b6289

It's not pretty, but it's fairly non-intrusive.

Please check it this meets your needs.

@wdoekes
Copy link
Member

wdoekes commented Apr 26, 2024

Looks good. Can you squash these changes?

@mstovenour mstovenour force-pushed the feat/explain-xml-syntax branch from c30132f to 78bb63e Compare April 26, 2024 22:26
@mstovenour
Copy link
Contributor Author

I fixed that. Sorry about all the noise. I was editing the docs directly in github and they make a commit for each file edit.

@wdoekes wdoekes merged commit 9c3a0bf into SIPp:master Apr 26, 2024
7 checks passed
@wdoekes
Copy link
Member

wdoekes commented Apr 26, 2024

Thanks!

@mstovenour mstovenour deleted the feat/explain-xml-syntax branch April 29, 2024 12:11
wdoekes added a commit that referenced this pull request Apr 29, 2024
Suggested and reviewed by Michael Stovenour (@mstovenour).
Closes: #721 (along with #725)
wdoekes added a commit that referenced this pull request Apr 29, 2024
Suggested and reviewed by Michael Stovenour (@mstovenour).
Closes: #721 (along with #725)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants