Skip to content

Conversation

@yokobond
Copy link
Contributor

Resolves

Fix for #1939 and partially #1077 .
Special characters '<>&'"' can be used in variable and message name.

Proposed Changes

aVariable.toXML() escapes special characters in the name.

Reason for Changes

Blocks in pallet and code pane build SVG string from XML presentation of variables. Variable name is used in blocks, so that the name of variables should be escaped special character in its XML.

Broadcast messages is a kind of variable in block presentation. Then this code fix the problem of broadcast message including special characters too.

Test Coverage

Added new unit test cases for engine/variable. It validates that toXML() returns escaped special characters '<>&'"'.
It contains tests for default specification too.

Integration test was not included, because these special characters are no harm in scratch-vm. This fix can be checked only on scratch-gui.

@thisandagain
Copy link
Contributor

/cc @ktbee

@thisandagain thisandagain added this to the February 2019 milestone Jan 28, 2019
@kchadha
Copy link
Contributor

kchadha commented Jan 31, 2019

@yokobond, thank you for submitting this pull request. We are actively working on the fix for #1077, and while your fix works for part of the issue, we are working on a larger change that might incorporate these changes as part of it.

Since we are still working on the fix for the larger issue, we will hold off on merging this pull request until we have figured out all the pieces of the larger issue to make sure these changes don't conflict with that work.

const htmlparser = require('htmlparser2');

test('spec', t => {
t.type(typeof Variable.SCALAR_TYPE, typeof Variable.LIST_TYPE);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary to fix now, but this test (and the one below) would probably be better written as the following:

Suggested change
t.type(typeof Variable.SCALAR_TYPE, typeof Variable.LIST_TYPE);
t.type(typeof Variable.SCALAR_TYPE, 'string');

We usually use the 'spec' test to verify the existence and types of properties on the class we are testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this. It must be string not only same type.
I overlooked that the return value is used by scratch-blocks as it is.

Copy link
Contributor

@kchadha kchadha left a comment

Choose a reason for hiding this comment

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

@yokobond thank you for making this pull request and for adding the tests! I left a minor suggestion in the comments, but I will merge this pull request as I would like to incorporate it into other changes I am making to resolve #1077.

@kchadha kchadha merged commit 166bc37 into scratchfoundation:develop Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants