Parser: Store attributes in unescaped JSON format #1213

dmsnell · 2017-06-16T14:44:26Z

Previously we borrowed the name="value" format of tag attributes from
HTML. This caused some headache with blocks because of the different
ways we are wanting to save data in the block comment headers, not the
least of which reason is due to how data must or need not be escaped.

In this change we overhaul the format of the data to allow for (mostly)
unescaped JSON data in the block comment headers. Minimal escaping
exists to prevent breaking the HTML comment and to prevent
poorly-implemented tools from messing up the comments as well.

nb · 2017-06-16T15:27:39Z

blocks/api/post.pegjs

-  const o = {};
-  o[ key ] = untransformValue( value );
-  return o;
+function maybeJSON( s ) {


Can we report an error somehow?

right now the error is reported as a null value. many things, including the string null is valid JSON. in that sense, I would like to propose that we only accept JSON objects, which would dramatically simplify the parser and semantics and serializer.

that would mean null can also signify an error, or alternatively, on error we could send the string of the content which failed to parse.

nb · 2017-06-16T16:49:48Z

Looks good, but it will break everything. Can we keep the old parser for a bit and use it as a fallback? On first save the format will be converted by the new serializer.

dmsnell · 2017-06-17T13:59:28Z

Looks good, but it will break everything. Can we keep the old parser for a bit and use it as a fallback? On first save the format will be converted by the new serializer.

thought this was why we are working on the prototype… 😉

westonruter · 2017-06-17T14:11:28Z

Beware. I'm rebasing this now to remove the merge commit.

westonruter · 2017-06-17T14:11:57Z

@dmsnell now that the plugin was just released on WordPress.org, the back-compat for attributes probably should be added.

westonruter · 2017-06-17T14:18:49Z

Rebased. Former HEAD is 0b00150.

dmsnell · 2017-06-17T14:25:59Z

lib/blocks.php

@@ -102,7 +103,7 @@ function do_blocks( $content ) {
 	global $wp_registered_blocks;

 	// Extract the blocks from the post content.
-	$matcher = '/<!--\s*wp:([a-z](?:[a-z0-9\/]+)*)\s+((?:(?!-->).)*)\s*\/?-->(?:.*?<!--\s*\/wp:\g1\s+-->)?/';
+	$matcher = '/<!--\s*wp:([a-z](?:[a-z0-9\/]+)*)\s+((?:(?!-->).)*?)\s*\/?-->(?:.*?<!--\s*\/wp:\g1\s+-->)?/';


we need to be extremely careful with combining wildcards. those +)* are troublesome, and I don't see why we are introducing *? - was it not working without the ?? @westonruter

@dmsnell The reason for this is the matches for a block like . Given that example, without the change on this line it was resulting in a match like {"poststoshow":3} /, which naturally is invalid JSON. So I was debating whether to rtrim the / or to make the pattern matching non-greedy. I decided on the latter, but you think the former is better then I'm happy to change.

dmsnell · 2017-06-17T14:27:16Z

@nb @jasmussen and @westonruter - I'm a bit mixed on "this will break things" don't we want to block all new usages of named attributes? should we really be providing a fallback to protect usage of the alpha release?

BE-Webdesign · 2017-06-17T14:40:20Z

Break it while we still can. I think the JSON pattern is a lot nicer.

westonruter · 2017-06-19T03:59:57Z

Rebased again. Former HEAD is b6411b0.

Build has been fixed. Tests have been updated.

I'd like to get this merged so we can move forward with dynamic blocks.

youknowriad · 2017-06-20T18:11:52Z

Yes, let's break the current blocks and release a plugin update quickly.

nylen · 2017-06-20T18:14:14Z

I think we should do this but I am a bit mixed on the breakage as well. I am happy to let someone else make the call here, I will just note that it shouldn't be too difficult to insert a temporary hack to convert string attributes to the new format.

jasmussen · 2017-06-20T19:22:17Z

I can understand the empathy for breakage, but we can't let the fact that we released the plugin into the repository hold us from making these necessary improvements, or even moving fast. This breakage should be expected under the beta moniker, and we even have an alert box on the publish button that warns that this a beta plugin.

We need to stay nimble, and not worry too much about breakage, especially in this phase, or we might get mired in additional challenges.

westonruter · 2017-06-20T19:36:25Z

I'll remove my back-compat PHP parsing and rebase.

westonruter · 2017-06-20T19:48:45Z

Rebased. Former HEAD was 79f3684.

aduth · 2017-06-21T15:24:09Z

post-content.js

 			'<p>The goal of this new editor is to make adding rich content to WordPress simple and enjoyable. This whole post is composed of <em>pieces of content</em>—somewhat similar to LEGO bricks—that you can move around and interact with. Move your cursor around and you\'ll notice the different blocks light up with outlines and arrows. Press the arrows to reposition blocks quickly, without fearing about losing things in the process of copying and pasting.</p>',
 			'<!-- /wp:core/text -->',

 			'<!-- wp:core/text -->',
 			'<p>What you are reading now is a <strong>text block</strong>, the most basic block of all. The text block has its own controls to be moved freely around the post...</p>',
 			'<!-- /wp:core/text -->',

-			'<!-- wp:core/text align="right" -->',
+			'<!-- wp:core/text { "align": "right" } -->',


Since default serialization wouldn't include whitespace between braces and after colon, should we similarly not do so here?

Since the post-content.js here is written by hand, it seems useful to have more spaces for readability when editing.

westonruter · 2017-06-21T16:31:55Z

Rebased to fix merge conflict. Former HEAD was bc9ee226.

westonruter · 2017-06-23T03:09:29Z

Re-re-rebased to fix merge conflicts. Former HEAD was 5d5c07a.

@mtias can this be merged?

westonruter · 2017-06-25T06:30:41Z

Resolving conflicts and re-basing again…

westonruter · 2017-06-25T06:49:32Z

Rebased. Former HEAD was bdf86f0.
Rebased. Former HEAD was f5900f6 (@dmsnell)

Previously we borrowed the `name="value"` format of tag attributes from HTML. This caused some headache with blocks because of the different ways we are wanting to save data in the block comment headers, not the least of which reason is due to how data must or need not be escaped. In this change we overhaul the format of the data to allow for (mostly) unescaped JSON data in the block comment headers. Minimal escaping exists to prevent breaking the HTML comment and to prevent poorly-implemented tools from messing up the comments as well.

dmsnell added the [Status] In Progress Tracking issues with work in progress label Jun 16, 2017

dmsnell requested review from nb, westonruter, youknowriad, mtias and aduth June 16, 2017 14:44

dmsnell mentioned this pull request Jun 16, 2017

[WIP] Try: Use JSON to encode block attribute values #1088

Closed

nb reviewed Jun 16, 2017

View reviewed changes

westonruter approved these changes Jun 16, 2017

View reviewed changes

westonruter force-pushed the parser/unescaped-json-attributes branch from 0b00150 to b6411b0 Compare June 17, 2017 14:17

dmsnell commented Jun 17, 2017

View reviewed changes

westonruter force-pushed the parser/unescaped-json-attributes branch from b6411b0 to cf056f6 Compare June 19, 2017 03:59

nylen mentioned this pull request Jun 20, 2017

Add PHP parser based on PEG.js grammar #1152

Merged

4 tasks

westonruter force-pushed the parser/unescaped-json-attributes branch from 79f3684 to 0004aaf Compare June 20, 2017 19:48

aduth approved these changes Jun 21, 2017

View reviewed changes

westonruter force-pushed the parser/unescaped-json-attributes branch from bc9ee22 to 5d5c07a Compare June 21, 2017 16:31

westonruter force-pushed the parser/unescaped-json-attributes branch from 5d5c07a to 4dd0f22 Compare June 23, 2017 03:08

westonruter added [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f and removed [Status] In Progress Tracking issues with work in progress labels Jun 23, 2017

westonruter force-pushed the parser/unescaped-json-attributes branch from bdf86f0 to f5900f6 Compare June 25, 2017 06:49

dmsnell and others added 13 commits June 26, 2017 18:17

Update serializer

14a5499

Add some example nested atributes

2260720

Use non-greedy attribute matching to exclude void block slash

b3acebb

Parse block attributes in PHP as JSON

300179f

Remove unused imported map

07a8cec

Fix tests by using JSON for block attributes

ef20146

Remove serializeValue function since obsolete

8d627e5

Add tests for serializeAttributes

d1a0a2e

Use JSON attribute syntax in phpunit tests

e9332d8

Remove obsolete parse_block_attributes()

5f32630

Fix typo in array literal

ac1dd7e

Update cover-image fixtures to use JSON attribute format

f63bbca

dmsnell force-pushed the parser/unescaped-json-attributes branch from f5900f6 to f63bbca Compare June 26, 2017 16:17

Update cover image attributes

7279467

dmsnell merged commit 49cfd17 into master Jun 26, 2017

dmsnell deleted the parser/unescaped-json-attributes branch June 26, 2017 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parser: Store attributes in unescaped JSON format #1213

Parser: Store attributes in unescaped JSON format #1213

dmsnell commented Jun 16, 2017 •

edited

Loading

nb Jun 16, 2017

dmsnell Jun 17, 2017

nb commented Jun 16, 2017

dmsnell commented Jun 17, 2017

westonruter commented Jun 17, 2017

westonruter commented Jun 17, 2017

westonruter commented Jun 17, 2017

dmsnell Jun 17, 2017

westonruter Jun 17, 2017

dmsnell commented Jun 17, 2017

BE-Webdesign commented Jun 17, 2017 •

edited

Loading

westonruter commented Jun 19, 2017

youknowriad commented Jun 20, 2017

nylen commented Jun 20, 2017

jasmussen commented Jun 20, 2017

westonruter commented Jun 20, 2017

westonruter commented Jun 20, 2017

aduth Jun 21, 2017

westonruter Jun 21, 2017

westonruter commented Jun 21, 2017

westonruter commented Jun 23, 2017

westonruter commented Jun 25, 2017

westonruter commented Jun 25, 2017 •

edited by dmsnell

Loading

Parser: Store attributes in unescaped JSON format #1213

Parser: Store attributes in unescaped JSON format #1213

Conversation

dmsnell commented Jun 16, 2017 • edited Loading

nb Jun 16, 2017

Choose a reason for hiding this comment

dmsnell Jun 17, 2017

Choose a reason for hiding this comment

nb commented Jun 16, 2017

dmsnell commented Jun 17, 2017

westonruter commented Jun 17, 2017

westonruter commented Jun 17, 2017

westonruter commented Jun 17, 2017

dmsnell Jun 17, 2017

Choose a reason for hiding this comment

westonruter Jun 17, 2017

Choose a reason for hiding this comment

dmsnell commented Jun 17, 2017

BE-Webdesign commented Jun 17, 2017 • edited Loading

westonruter commented Jun 19, 2017

youknowriad commented Jun 20, 2017

nylen commented Jun 20, 2017

jasmussen commented Jun 20, 2017

westonruter commented Jun 20, 2017

westonruter commented Jun 20, 2017

aduth Jun 21, 2017

Choose a reason for hiding this comment

westonruter Jun 21, 2017

Choose a reason for hiding this comment

westonruter commented Jun 21, 2017

westonruter commented Jun 23, 2017

westonruter commented Jun 25, 2017

westonruter commented Jun 25, 2017 • edited by dmsnell Loading

dmsnell commented Jun 16, 2017 •

edited

Loading

BE-Webdesign commented Jun 17, 2017 •

edited

Loading

westonruter commented Jun 25, 2017 •

edited by dmsnell

Loading