Skip to content
This repository was archived by the owner on Apr 8, 2020. It is now read-only.

A fix for some errors in escapeXML() #54

Open
MarkMaldaba wants to merge 2 commits into
base: master
Choose a base branch
from
Open

A fix for some errors in escapeXML() #54

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6dc291f
Fixed an issue in escapeXML() whereby if the supplied value is undefi…
MarkMaldaba Mar 5, 2018
bcddef1
I've updated escapeXML() so that it properly escapes ampersands.
MarkMaldaba Mar 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions raphael.export.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,29 @@
function escapeXML(s) {
if ( typeof s === 'number' ) return s.toString();

var replace = { '<': 'lt', '>': 'gt', '"': 'quot', '\'': 'apos' };
// We wrap the check in a try/catch block. If an error occurs then the
// function was passed a non-string value or a value that otherwise
// couldn't be converted to a string. In this case, a JavaScript error
// will be thrown, and our catch block will return an empty string.
try {
var replace = { '&': 'amp', '<': 'lt', '>': 'gt', '"': 'quot', '\'': 'apos' };

// Ensure s is a string, by converting it using toString(), if possible.
// If this method doesn't exist, then an error will be thrown and our
// catch block will return an empty string.
// Note that (perhaps, surprisingly) string values have a toString()
// function, so we don't need to check the type before calling.
s = s.toString();
Copy link
Owner

@AliasIO AliasIO Mar 9, 2018

Choose a reason for hiding this comment

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

How about s = s.hasOwnProperty('toString') ? s.toString() : '' instead of a try/catch block?

Copy link
Author

@MarkMaldaba MarkMaldaba Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That won't work if passed an object that inherits toString() from the prototype chain, e.g. something that extends the String class. You could use typeof s.toString == 'undefined', except I think that breaks if s is undefined (at least, it did on IE8) so you would also need to check that It was at this point I decided to play it safe and go with a try/catch block, in case there were other values that would also result in errors in this situation (e.g. null).

Maybe this level of rigour doesn't matter for this application as it may be that we always have either a string a number or undefined, but I didn't want to take the risk, so a try/catch seemed safer.

Copy link
Owner

@AliasIO AliasIO Mar 10, 2018

Choose a reason for hiding this comment

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

In that case s = s && s.toString ? s.toString() : '' will do. Why would toString not work if the object extends the String class? Any object that inherits from Object.prototype will have a toString method, unless it was deliberately removed for some odd reason.

I think we should avoid try/catch and handle these cases explicitly.

Copy link
Author

@MarkMaldaba MarkMaldaba Mar 11, 2018

Choose a reason for hiding this comment

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

In that case s = s && s.toString ? s.toString() : '' will do.

Depends how thorough you want the function to be. With that code, a value of false will result in "" whereas true will result in "true". In my version, you would get "false", which is more consistent and correct.

However, as I said, if we don't care about that kind of edge-case, which is probably not likely to occur in the context of what the plugin is doing, then your suggested code would be good enough.

Personally, I don't like using inline if statements that are more complex than individual variable references and literals. It becomes very hard to read, parse and understand as soon as you introduce more complex expressions. I would expand it into a standard if/else block in this situation.

However, it's your plugin so I'll happy implement it whatever way you like - please advise.

Why would toString not work if the object extends the String class?

s.hasOwnProperty('toString') won't work, as the property comes from the prototype chain. Checking s.toString directly as per your second suggestion, should work fine.

Copy link
Author

@MarkMaldaba MarkMaldaba Jul 16, 2018

Choose a reason for hiding this comment

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

Hi @AliasIO - I would be keen to resolve this PR, and made a couple of alternative suggestions above. Please let me know which solution you prefer and I'll update the PR to meet your preference.


for ( var entity in replace ) {
s = s.replace(new RegExp(entity, 'g'), '&' + replace[entity] + ';');
}

for ( var entity in replace ) {
s = s.replace(new RegExp(entity, 'g'), '&' + replace[entity] + ';');
}
return s;

return s;
} catch ( e ) {
return "";
}
}

/**
Expand Down