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

Code Block should not render embeds (or shortcodes) #13996

Merged
merged 12 commits into from May 2, 2019

Conversation

@davilera
Copy link
Contributor

commented Feb 21, 2019

Description

Fixes #13927 - "The Code Block renders embeds". As OP said:

I expected the code block to display every character of (pseudo) code inside it. Nothing should render at all.

Not only should block codes not render embeds, but also they should not render shortcodes.

How has this been tested?

I've reproduced the bug described in #13927.

The PR prevents shortcodes from running by changing an embed's opening character [ to [.

It also prevents isolated URLs from becoming an embed by changing https:// to https://.

Types of changes

Bug fix. In a code block:

  • & is saved as &.
  • [ is saved as [.
  • / is saved as / (in isolated URLs only).

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
David Aguilera
@aduth
Copy link
Member

left a comment

Part of me wonders if this is not something the server should be considering when evaluating whether to transform an embed or shortcode.

Namely, even after these changes, I'd still be able to produce fascinating results when manually crafting a post with the content:

<pre><code>

https://www.youtube.com/watch?v=YuOBzWF0Aws

</code></pre>
Show resolved Hide resolved packages/block-library/src/code/index.js Outdated
Show resolved Hide resolved packages/block-library/src/code/index.js Outdated
Show resolved Hide resolved packages/block-library/src/code/index.js Outdated

David Aguilera added some commits Feb 22, 2019

@davilera

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Part of me wonders if this is not something the server should be considering when evaluating whether to transform an embed or shortcode.

I agree. AFAIK, though, WordPress transforms embeds/shortcodes in the post content ignoring the fact that they might be in a block.

If you feel like tweaking code block's save method isn't a good idea, I'd recommend a different approach: run a filter in the post content with a higher priority and escape the characters before embed/shortcodes run. That is, doing the same this PR does, but in PHP and performing the escape on the fly (without committing them to the DB).

This alternative solution, however, would require a few more additional changes: we would also need to add a new filter or extend the blocks API so that developers could specify which blocks accept embed/shortcodes and which blocks don't.

So, how should I/we proceed?

@aduth
Copy link
Member

left a comment

So, how should I/we proceed?

I think as implemented here is fine.

Worth noting, however, that it relies on the specific behavior of @wordpress/element's renderToString behavior departing from that of React, where React.renderToStaticMarkup may result in double-escaping if given the block element input here.

⇒ node       
> var { renderToStaticMarkup } = require( 'react-dom/server' );
> var { createElement, renderToString } = require( '@wordpress/element' );
> renderToStaticMarkup( createElement( 'code', null, '&#91;myshortcode/]' ) );
'<code>&amp;#91;myshortcode/]</code>'
> renderToString( createElement( 'code', null, '&#91;myshortcode/]' ) );
'<code>&#91;myshortcode/]</code>'

It's not strictly an issue, but may become problematic if the behavior changes over time. I might like to see some tests here to avoid future regressions to this point, or even just enhancing / adding additional variations to the existing code block fixtures:

https://github.com/WordPress/gutenberg/blob/master/packages/e2e-tests/fixtures/blocks/core__code.html

(Running npm run fixtures:regenerate after changing / adding new fixtures)

Show resolved Hide resolved packages/block-library/src/code/index.js Outdated

@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019

David Aguilera added some commits Mar 29, 2019

@davilera

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Thanks for all the tips, @aduth. Finally, I've decided to follow a different approach.

Instead of escaping shortcode and link characters in save, I'm now escaping them when CodeEdit runs setAttributes. This allows me to undo this action when the content is about to be shown in CodeEdit.

I think this solution fixes the issue that you said could arise in the future, and it's probably more elegant.

What do you think?

@aduth
Copy link
Member

left a comment

Apologies for the delay in revisiting this.

I think the general approach seems agreeable. There's certainly some prior art in this sort of "value dance", like seen in the preformatted block.

I observed an issue in testing this with a code block consisting of newlines + YouTube URL, noted in the review comments.

Show resolved Hide resolved packages/block-library/src/code/utils.js Outdated

David Aguilera added some commits Apr 16, 2019

@davilera

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

I created a few unit tests in commit 1cb6ff8. Oh, and I then renamed the test file from index.js to utils.js in 03e618d.

Is there anything left?

@aduth

aduth approved these changes May 2, 2019

Copy link
Member

left a comment

This looks great and works well in my testing 👍

I'd considered whether overlap with the escape name in the JavaScript specification may be of concern (reference). While it could be a problem for future maintainability on the point of confusion around whether the symbols refer to the global or the imported value, it doesn't have an effective impact as long as the imported values are in scope. For that reason, I'd not consider it to be a blocker.

Thanks for your patience in the reviews on this one.

@aduth aduth merged commit 92e73e8 into WordPress:master May 2, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.