Skip to content

Block API: Replace JSON-escaped quotation mark with unicode escape sequence #6619

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.

Sign up for GitHub

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

Merged
gziolo merged 2 commits into from
Jun 21, 2018
Merged

Block API: Replace JSON-escaped quotation mark with unicode escape sequence #6619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c55552
Block API: Replace JSON-escaped quotation mark with unicode escape se…
aduth May 7, 2018
218370a
Parser: Test full lifecycle of quote-escaped attributes parse
aduth May 10, 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
29 changes: 23 additions & 6 deletions blocks/api/serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,29 @@ export function getCommentAttributes( allAttributes, blockType ) {
return attributes;
}

export function serializeAttributes( attrs ) {
return JSON.stringify( attrs )
.replace( /--/g, '\\u002d\\u002d' ) // don't break HTML comments
.replace( /</g, '\\u003c' ) // don't break standard-non-compliant tools
.replace( />/g, '\\u003e' ) // ibid
.replace( /&/g, '\\u0026' ); // ibid
/**
* Given an attributes object, returns a string in the serialized attributes
* format prepared for post content.
*
* @param {Object} attributes Attributes object.
*
* @return {string} Serialized attributes.
*/
export function serializeAttributes( attributes ) {
return JSON.stringify( attributes )
// Don't break HTML comments.
.replace( /--/g, '\\u002d\\u002d' )

// Don't break non-standard-compliant tools.
.replace( /</g, '\\u003c' )
.replace( />/g, '\\u003e' )
.replace( /&/g, '\\u0026' )

// Bypass server stripslashes behavior which would unescape stringify's
// escaping of quotation mark.
//
// See: https://developer.wordpress.org/reference/functions/wp_kses_stripslashes/
.replace( /\\"/g, '\\u0022' );
Copy link
Member

@dmsnell dmsnell May 7, 2018

Choose a reason for hiding this comment

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

Can we find a corresponding translation in the parser? I'm nervous about introducing asymmetries in the parser/printer system that could confuse people or introduce inconsistencies. For example, what happens if we want to write \" in a code block? Would it be preserved or transformed into \u0022?

Is there a way we can transform the quotation mark on save so that it never gets mangled by the WordPress backend?

Copy link
Member Author

@aduth aduth May 7, 2018

Choose a reason for hiding this comment

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

I'm nervous about introducing asymmetries in the parser/printer system that could confuse people or introduce inconsistencies.

To be fair, is it being introduced? What about the other replacements here? I ask partly because I was hoping to find precedent in the parser 😄

Is there a way we can transform the quotation mark on save so that it never gets mangled by the WordPress backend?

I don't have the knowledge to speak to whether it's viable, but the documentation of the wp_kses_stripslashes function reads like a hacky fix ("It’s really weird, but the quoting from preg_replace(//e) seems to require this") that could potentially do for a better solution more accommodating of the slash'd quote.

Copy link
Member

@dmsnell dmsnell May 7, 2018

Choose a reason for hiding this comment

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

To be fair, is it being introduced? What about the other replacements here? I ask partly because I was hoping to find precedent in the parser 😄

Huh. Well in my head there was precedence. Maybe it was primarily the HTML itself which was the other half of the equation.

If we can store that value in a code block and have it remain the same through the whole cycle then I think we're fine.

/me digs around to find those unserializers…

Copy link
Member Author

@aduth aduth May 10, 2018

Choose a reason for hiding this comment

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

By default this wouldn't impact a code block, since it's only relevant for the JSON-serialized attributes. The code block sources its content from the markup. Manually updating the block to use comment attributes, the escaped form becomes:

<!-- wp:code {"content":"$foo = \u0022my \\\u0022escaped\\\u0022 string\u0022;"} -->
<pre class="wp-block-code"><code>$foo = "my \"escaped\" string";</code></pre>
<!-- /wp:code -->

Which apparently the parser converts back to its non-unicode form when restored:

{
  "blockName": "core/code",
  "attrs": {
    "content": "$foo = \"my \\\"escaped\\\" string\";"
  },
  "innerBlocks": [],
  "innerHTML": "\n<pre class=\"wp-block-code\"><code>$foo = \"my \\\"escaped\\\" string\";</code></pre>\n"
}

Further interesting to note is that the original problem (slash stripping) doesn't exist with the code block as implemented currently. I think it has to do with the behavior of wp_kses_split which only operates on text within HTML comments (i.e. serialized block attributes) or within the opening tags, not the content between the opening and closing tag.

Copy link
Member Author

@aduth aduth May 10, 2018

Choose a reason for hiding this comment

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

Let me see if it'll be simple enough to write a unit test for this.

Copy link
Member Author

@aduth aduth May 10, 2018

Choose a reason for hiding this comment

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

Added in cc34b0e.

}

/**
Expand Down
21 changes: 21 additions & 0 deletions blocks/api/test/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import {
getBlockTypes,
setUnknownTypeHandlerName,
} from '../registration';
import { createBlock } from '../factory';
import serialize from '../serializer';

describe( 'block parser', () => {
const defaultBlockSettings = {
Expand Down Expand Up @@ -571,5 +573,24 @@ describe( 'block parser', () => {
'core/test-block', 'core/void-block',
] );
} );

it( 'should parse with unicode escaped returned to original representation', () => {
registerBlockType( 'core/code', {
category: 'common',
title: 'Code Block',
attributes: {
content: {
type: 'string',
},
},
save: ( { attributes } ) => attributes.content,
} );

const content = '$foo = "My \"escaped\" text.";';
const block = createBlock( 'core/code', { content } );
const serialized = serialize( block );
const parsed = parse( serialized );
expect( parsed[ 0 ].attributes.content ).toBe( content );
} );
} );
} );
7 changes: 7 additions & 0 deletions blocks/api/test/serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,15 +130,22 @@ describe( 'block serializer', () => {
it( 'should not break HTML comments', () => {
expect( serializeAttributes( { a: '-- and --' } ) ).toBe( '{"a":"\\u002d\\u002d and \\u002d\\u002d"}' );
} );

it( 'should not break standard-non-compliant tools for "<"', () => {
expect( serializeAttributes( { a: '< and <' } ) ).toBe( '{"a":"\\u003c and \\u003c"}' );
} );

it( 'should not break standard-non-compliant tools for ">"', () => {
expect( serializeAttributes( { a: '> and >' } ) ).toBe( '{"a":"\\u003e and \\u003e"}' );
} );

it( 'should not break standard-non-compliant tools for "&"', () => {
expect( serializeAttributes( { a: '& and &' } ) ).toBe( '{"a":"\\u0026 and \\u0026"}' );
} );

it( 'should replace quotation marks', () => {
expect( serializeAttributes( { a: '" and "' } ) ).toBe( '{"a":"\\u0022 and \\u0022"}' );
} );
} );

describe( 'getCommentDelimitedContent()', () => {
Expand Down