Fix breaking preprocessors' directives when parsing attributes #171

yeegor · 2021-01-13T13:11:24Z

Overview

The issue described above has been found during the development of the create-scandipwa-app.

Motivation

See the following code example. It is located in a PHP file and is valid: the corresponding function is called when the file is parsed. This has been tested using the Magento 2 framework, nonetheless, it should be possible to recreate this without the framework.

<html lang="<?= $this->getLocaleCode() ?>">

The code fragment above, when parsed back and forth, is expected to remain the same. But in fact, it turns into the following code.

<html lang="&lt;?= $this->getLocaleCode() ?>">

Context

The used version is 0.4.0
The parsing process has been invoked as follows:

const xmldom = require('xmldom');

const domParser = new xmldom.DOMParser();
const xmlSerializer = new xmldom.XMLSerializer();

const templateFile = ...

const parseToDOM = (code) => domParser.parseFromString(code, 'text/html');
const parseToString = (dom) => xmlSerializer.serializeToString(dom);

// This result is broken, as described above
const result = parseToString(parseToDOM(templateFile));

Description

This PR removes < from mandatory-to-replace token list for attribute parsing process.

karfau · 2021-01-14T21:06:41Z

I thing we have multiple related issues with entities and roundtrips and this looks like a very valid (but of course breaking) fix for this specific subset of situations described.

Can you run the tests and tell jest to update the snapshots and also adopt any other failing test? Thx.

…-in-attributes

karfau · 2021-01-19T03:15:37Z

The only thing that might be concerning is that with this change the following:

<doc a1="&quot;&lt;&amp;&gt;&apos;"></doc>

will be serialized to

<doc a1="&quot;<&amp;>'"/>

(which is consistent with the already existing behavior for > and ')

@brodybits what do you think?

brodybits · 2021-01-19T03:22:38Z

I wonder if we should just keep the quoted attribute value unchanged.

karfau · 2021-01-19T04:18:08Z

I wonder if we should just keep the quoted attribute value unchanged.

I think we should be able to add this as an option to Node.toString/Document.toString() but it would not be something that is part of the standard. And maybe this can rather happen in a different PR?

brodybits · 2021-01-25T16:34:48Z

No more objections on my part. Thanks.

Fix breaking preprocessors' directives when parsing attributes

370ef4a

karfau added 2 commits January 19, 2021 04:05

Merge remote-tracking branch 'upstream/master' into broken-processing…

ffe4f74

…-in-attributes

test: Update jest snapshots

48c9264

karfau requested a review from brodybits January 19, 2021 03:16

karfau added this to the 0.5.0 milestone Jan 21, 2021

karfau merged commit fd47c51 into xmldom:master Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix breaking preprocessors' directives when parsing attributes #171

Fix breaking preprocessors' directives when parsing attributes #171

yeegor commented Jan 13, 2021

karfau commented Jan 14, 2021

karfau commented Jan 19, 2021 •

edited

brodybits commented Jan 19, 2021

karfau commented Jan 19, 2021

brodybits commented Jan 25, 2021

Fix breaking preprocessors' directives when parsing attributes #171

Fix breaking preprocessors' directives when parsing attributes #171

Conversation

yeegor commented Jan 13, 2021

Overview

Motivation

Context

Description

karfau commented Jan 14, 2021

karfau commented Jan 19, 2021 • edited

brodybits commented Jan 19, 2021

karfau commented Jan 19, 2021

brodybits commented Jan 25, 2021

karfau commented Jan 19, 2021 •

edited