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

Make "js" escaped strings embeddable in JSON #724

Merged
merged 1 commit into from
Mar 9, 2020

Conversation

dorian-marchal
Copy link
Contributor

@dorian-marchal dorian-marchal commented Feb 24, 2020

The "js" escape strategy of Twig PHP produces a JSON compatible escaping.
This is useful as it allows embedding JS escaped strings in JSON script.

For example, this can be used to templates JSON-LD scripts:

<script type="application/ld+json">
{
  "@context": "http://schema.org/",
  "@type": "Person",
  "name": "{{ name }}",
}
</script>

Unfortunately, the twig.js implementation doesn't produce JSON compatible escaping.

For example:

const { twig } = require('twig');
const compiledTemplate = twig({ autoescape: 'js', data: '{{ foo }}' });
compiledTemplate.render({ foo: '<foo>' });

// > \x3Cfoo\x3E

\x3Cfoo\x3E is not valid JSON and can't be parsed by some JSON parsers.

With the same template/data, Twig PHP returns \u003Cfoo\u003E:

<?php
$twig = new \Twig\Environment(new \Twig\Loader\ArrayLoader(), ['autoescape' => 'js']);
$twig->createTemplate('{{ foo }}')->render(['foo' => '<foo>']);

// > \u003Cfoo\u003E

In TwigPHP, The "js" escape strategy is defined there: https://github.com/twigphp/Twig/blob/6384ba6c3ca7695ef0aec93bdf3b55c9bb18b5de/src/Extension/EscaperExtension.php#L247-L295

Note: This PR replaces JSON-incompatible hexadecimal escape sequences (\x..) with JSON-compatible unicode escape sequences (\u00..).
It also prevents the escaping of some chars that are not escaped by TwigPHP.
This fixes specific use cases, but it doesn't make the escaping fully compatible with TwigPHP's.

I noticed that twing outputs the same string as TwigPHP.
Maybe you can help us, here, @ericmorand?

@ericmorand
Copy link
Contributor

@dorian-marchal I can help yes. The js escaping strategy is implemented here in Twing:

https://github.com/NightlyCommit/twing/blob/cb934c305bf68e8e507918275d477a1f7a45b387/src/lib/extension/core/filters/escape.ts#L52

It has a dependency on bin2hex though, provided by Locutus PHP. I don't think that's a big issue.

@dorian-marchal
Copy link
Contributor Author

@ericmorand Thanks. 👍

I see there is also a dependency on utf8-binary-cutter.

Both packages seem quite big:

Twig.js probably doesn't work well for the edge cases these libs handle compared to JavaScript's native functions but maybe we could do without them. I believe Twig.js doesn't try to be fully compatible with Twig PHP.

What do you think @RobLoach? Does this PR look OK to you as is?

@dorian-marchal dorian-marchal changed the title [WIP] Make "js" escaped strings embeddable in JSON Make "js" escaped strings embeddable in JSON Feb 27, 2020
@dorian-marchal dorian-marchal force-pushed the fix-js-escaping branch 3 times, most recently from ca53ff9 to fb5dac6 Compare February 27, 2020 13:29
The "js" escape strategy of Twig PHP [0] produces a JSON compatible escaping.
This is useful as it allows embedding JS escaped strings in JSON script.

Note: This commit replaces JSON-incompatible hexadecimal escape
sequences (\x..) with JSON-compatible unicode escape sequences (\u00..).
It also prevents the escaping of some chars that are not escaped by TwigPHP.
This fixes specific use cases, but it doesn't make the escaping fully
compatible with TwigPHP's.

[0]: https://git.io/JvEGD
@dorian-marchal
Copy link
Contributor Author

I rebased this PR on master. I'm available if you have questions about it @RobLoach. 👍

@RobLoach
Copy link
Collaborator

RobLoach commented Mar 9, 2020

Thanks! Looks worth merging. If there's any follow ups, we can get them in a future PR. 👍

@RobLoach RobLoach merged commit 7ab61e9 into twigjs:master Mar 9, 2020
@dorian-marchal dorian-marchal deleted the fix-js-escaping branch March 9, 2020 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants