Skip to content

Commit

Permalink
Parser: Fix JS/PHP inconsistencies with empty attributes (WordPress#1…
Browse files Browse the repository at this point in the history 
…1434)

This patch updates a couple of inconsistent behaviors when dealing with
empty attributes and with the way PHP and JS treat empty objects differently.

## PHP empty attributes
When returning blocks without attributes the spec PHP parser has been
sending `[]` instead of `{}` due to complications of how PHP serializes
empty arrays to JSON.

In this patch we're adding a new test to verify the behavior and then
fixing the spec parser so that the output from the PHP version matches
the output from the JS version identically.

## Bug with empty attributes in comments
The default parser introduced a bug where `<!-- wp:anything {} /-->` would
fail to parse because it expected some content inside the curly brackets for
the JSOn attributes.

This is fixed in this patch by changing the `+?` quantifier in the RegExp
tokenizer with a `*?` to allow for empty attributes.

A test suite has been added to verify that when we parse an array of different
block formulations that they produce what we expect. A bug in a test was the
reason I didn't originally find this bug.
  • Loading branch information
@dmsnell
dmsnell authored Nov 10, 2018
1 parent 50380b3 commit fdb8add
Show file tree
Hide file tree
Showing 8 changed files with 136 additions and 86 deletions.
22 changes: 17 additions & 5 deletions lib/parser.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ private function peg_f2($blockName, $a) { return $a; }
private function peg_f3($blockName, $attrs) {
return array(
'blockName' => $blockName,
'attrs' => isset( $attrs ) ? $attrs : array(),
'attrs' => empty( $attrs ) ? peg_empty_attrs() : $attrs,
'innerBlocks' => array(),
'innerHTML' => '',
'innerContent' => array(),
Expand All @@ -271,7 +271,7 @@ private function peg_f4($s, $children, $e) {

return array(
'blockName' => $s['blockName'],
'attrs' => $s['attrs'],
'attrs' => empty( $s['attrs'] ) ? peg_empty_attrs() : $s['attrs'],
'innerBlocks' => $innerBlocks,
'innerHTML' => $innerHTML,
'innerContent' => $innerContent,
Expand Down Expand Up @@ -1582,6 +1582,18 @@ public function parse($input) {
// The `maybeJSON` function is not needed in PHP because its return semantics
// are the same as `json_decode`

if ( ! function_exists( 'peg_empty_attrs' ) ) {
function peg_empty_attrs() {
static $empty_attrs = null;

if ( null === $empty_attrs ) {
$empty_attrs = json_decode( '{}', true );
}

return $empty_attrs;
}
}

// array arguments are backwards because of PHP
if ( ! function_exists( 'peg_process_inner_content' ) ) {
function peg_process_inner_content( $array ) {
Expand Down Expand Up @@ -1610,7 +1622,7 @@ function peg_join_blocks( $pre, $tokens, $post ) {
if ( ! empty( $pre ) ) {
$blocks[] = array(
'blockName' => null,
'attrs' => array(),
'attrs' => peg_empty_attrs(),
'innerBlocks' => array(),
'innerHTML' => $pre,
'innerContent' => array( $pre ),
Expand All @@ -1625,7 +1637,7 @@ function peg_join_blocks( $pre, $tokens, $post ) {
if ( ! empty( $html ) ) {
$blocks[] = array(
'blockName' => null,
'attrs' => array(),
'attrs' => peg_empty_attrs(),
'innerBlocks' => array(),
'innerHTML' => $html,
'innerContent' => array( $html ),
Expand All @@ -1636,7 +1648,7 @@ function peg_join_blocks( $pre, $tokens, $post ) {
if ( ! empty( $post ) ) {
$blocks[] = array(
'blockName' => null,
'attrs' => array(),
'attrs' => peg_empty_attrs(),
'innerBlocks' => array(),
'innerHTML' => $post,
'innerContent' => array( $post ),
Expand Down
25 changes: 17 additions & 8 deletions packages/block-serialization-default-parser/parser.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,14 @@ class WP_Block_Parser {
*/
public $stack;

/**
* Empty associative array, here due to PHP quirks
*
* @since 4.4.0
* @var array empty associative array
*/
public $empty_attrs;

/**
* Parses a document and returns a list of block structures
*
Expand All @@ -186,10 +194,11 @@ class WP_Block_Parser {
* @return WP_Block_Parser_Block[]
*/
function parse( $document ) {
$this->document = $document;
$this->offset = 0;
$this->output = array();
$this->stack = array();
$this->document = $document;
$this->offset = 0;
$this->output = array();
$this->stack = array();
$this->empty_attrs = json_decode( '{}', true );

do {
// twiddle our thumbs
Expand Down Expand Up @@ -364,7 +373,7 @@ function next_token() {
* match back in PHP to see which one it was.
*/
$has_match = preg_match(
'/<!--\s+(?<closer>\/)?wp:(?<namespace>[a-z][a-z0-9_-]*\/)?(?<name>[a-z][a-z0-9_-]*)\s+(?<attrs>{(?:[^}]+|}+(?=})|(?!}\s+-->).)+?}\s+)?(?<void>\/)?-->/s',
'/<!--\s+(?<closer>\/)?wp:(?<namespace>[a-z][a-z0-9_-]*\/)?(?<name>[a-z][a-z0-9_-]*)\s+(?<attrs>{(?:[^}]+|}+(?=})|(?!}\s+-->).)*?}\s+)?(?<void>\/)?-->/s',
$this->document,
$matches,
PREG_OFFSET_CAPTURE,
Expand Down Expand Up @@ -392,7 +401,7 @@ function next_token() {
*/
$attrs = $has_attrs
? json_decode( $matches[ 'attrs' ][ 0 ], /* as-associative */ true )
: json_decode( '{}', /* don't ask why, just verify in PHP */ false );
: $this->empty_attrs;

/*
* This state isn't allowed
Expand Down Expand Up @@ -422,8 +431,8 @@ function next_token() {
* @param string $innerHTML HTML content of block
* @return WP_Block_Parser_Block freeform block object
*/
static function freeform( $innerHTML ) {
return new WP_Block_Parser_Block( null, array(), array(), $innerHTML, array( $innerHTML ) );
function freeform( $innerHTML ) {
return new WP_Block_Parser_Block( null, $this->empty_attrs, array(), $innerHTML, array( $innerHTML ) );
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/block-serialization-default-parser/src/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ let document;
let offset;
let output;
let stack;
const tokenizer = /<!--\s+(\/)?wp:([a-z][a-z0-9_-]*\/)?([a-z][a-z0-9_-]*)\s+({(?:[^}]+|}+(?=})|(?!}\s+-->)[^])+?}\s+)?(\/)?-->/g;
const tokenizer = /<!--\s+(\/)?wp:([a-z][a-z0-9_-]*\/)?([a-z][a-z0-9_-]*)\s+({(?:[^}]+|}+(?=})|(?!}\s+-->)[^])*?}\s+)?(\/)?-->/g;

function Block( blockName, attrs, innerBlocks, innerHTML, innerContent ) {
return {
Expand Down
29 changes: 0 additions & 29 deletions packages/block-serialization-default-parser/test/__snapshots__/index.js.snap
View file

This file was deleted.

22 changes: 17 additions & 5 deletions packages/block-serialization-spec-parser/grammar.pegjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,18 @@
// The `maybeJSON` function is not needed in PHP because its return semantics
// are the same as `json_decode`
if ( ! function_exists( 'peg_empty_attrs' ) ) {
function peg_empty_attrs() {
static $empty_attrs = null;
if ( null === $empty_attrs ) {
$empty_attrs = json_decode( '{}', true );
}
return $empty_attrs;
}
}
// array arguments are backwards because of PHP
if ( ! function_exists( 'peg_process_inner_content' ) ) {
function peg_process_inner_content( $array ) {
Expand Down Expand Up @@ -78,7 +90,7 @@ if ( ! function_exists( 'peg_join_blocks' ) ) {
if ( ! empty( $pre ) ) {
$blocks[] = array(
'blockName' => null,
'attrs' => array(),
'attrs' => peg_empty_attrs(),
'innerBlocks' => array(),
'innerHTML' => $pre,
'innerContent' => array( $pre ),
Expand All @@ -93,7 +105,7 @@ if ( ! function_exists( 'peg_join_blocks' ) ) {
if ( ! empty( $html ) ) {
$blocks[] = array(
'blockName' => null,
'attrs' => array(),
'attrs' => peg_empty_attrs(),
'innerBlocks' => array(),
'innerHTML' => $html,
'innerContent' => array( $html ),
Expand All @@ -104,7 +116,7 @@ if ( ! function_exists( 'peg_join_blocks' ) ) {
if ( ! empty( $post ) ) {
$blocks[] = array(
'blockName' => null,
'attrs' => array(),
'attrs' => peg_empty_attrs(),
'innerBlocks' => array(),
'innerHTML' => $post,
'innerContent' => array( $post ),
Expand Down Expand Up @@ -212,7 +224,7 @@ Block_Void
/** <?php
return array(
'blockName' => $blockName,
'attrs' => isset( $attrs ) ? $attrs : array(),
'attrs' => empty( $attrs ) ? peg_empty_attrs() : $attrs,
'innerBlocks' => array(),
'innerHTML' => '',
'innerContent' => array(),
Expand All @@ -236,7 +248,7 @@ Block_Balanced
return array(
'blockName' => $s['blockName'],
'attrs' => $s['attrs'],
'attrs' => empty( $s['attrs'] ) ? peg_empty_attrs() : $s['attrs'],
'innerBlocks' => $innerBlocks,
'innerHTML' => $innerHTML,
'innerContent' => $innerContent,
Expand Down
22 changes: 17 additions & 5 deletions packages/block-serialization-spec-parser/parser.js
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

71 changes: 67 additions & 4 deletions packages/block-serialization-spec-parser/shared-tests.js
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,64 @@
export const jsTester = ( parse ) => () => {
describe( 'basic parsing', () => {
test( 'parse() works properly', () => {
expect( parse( '<!-- wp:core/more --><!--more--><!-- /wp:core/more -->' ) ).toMatchSnapshot();
describe( 'output structure', () => {
test( 'output is an array', () => {
expect( parse( '' ) ).toEqual( expect.any( Array ) );
expect( parse( 'test' ) ).toEqual( expect.any( Array ) );
expect( parse( '<!-- wp:void /-->' ) ).toEqual( expect.any( Array ) );
expect( parse( '<!-- wp:block --><!-- wp:inner /--><!-- /wp:block -->' ) ).toEqual( expect.any( Array ) );
expect( parse( '<!-- wp:first /--><!-- wp:second /-->' ) ).toEqual( expect.any( Array ) );
} );

test( 'parses blocks of various types', () => {
expect( parse( '<!-- wp:void /-->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/void' );
expect( parse( '<!-- wp:void {} /-->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/void' );
expect( parse( '<!-- wp:void {"value":true} /-->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/void' );
expect( parse( '<!-- wp:void {"a":{}} /-->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/void' );
expect( parse( '<!-- wp:void { "value" : true } /-->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/void' );
expect( parse( '<!-- wp:void {\n\t"value" : true\n} /-->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/void' );
expect( parse( '<!-- wp:block --><!-- /wp:block -->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/block' );
expect( parse( '<!-- wp:block {} --><!-- /wp:block -->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/block' );
expect( parse( '<!-- wp:block {"value":true} --><!-- /wp:block -->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/block' );
expect( parse( '<!-- wp:block {} -->inner<!-- /wp:block -->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/block' );
expect( parse( '<!-- wp:block {"value":{"a" : "true"}} -->inner<!-- /wp:block -->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/block' );
} );

test( 'blockName is namespaced string (except freeform)', () => {
expect( parse( 'freeform has null name' )[ 0 ] ).toHaveProperty( 'blockName', null );
expect( parse( '<!-- wp:more /-->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/more' );
expect( parse( '<!-- wp:core/more /-->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/more' );
expect( parse( '<!-- wp:my/more /-->' )[ 0 ] ).toHaveProperty( 'blockName', 'my/more' );
} );

test( 'JSON attributes are key/value object', () => {
expect( parse( 'freeform has empty attrs' )[ 0 ] ).toHaveProperty( 'attrs', {} );
expect( parse( '<!-- wp:void /-->' )[ 0 ] ).toHaveProperty( 'attrs', {} );
expect( parse( '<!-- wp:void {} /-->' )[ 0 ] ).toHaveProperty( 'blockName', 'core/void' );
expect( parse( '<!-- wp:void {} /-->' )[ 0 ] ).toHaveProperty( 'attrs', {} );
expect( parse( '<!-- wp:void {"key": "value"} /-->' )[ 0 ] ).toHaveProperty( 'attrs', { key: 'value' } );
expect( parse( '<!-- wp:block --><!-- /wp:block -->' )[ 0 ] ).toHaveProperty( 'attrs', {} );
expect( parse( '<!-- wp:block {} --><!-- /wp:block -->' )[ 0 ] ).toHaveProperty( 'attrs', {} );
expect( parse( '<!-- wp:block {"key": "value"} --><!-- /wp:block -->' )[ 0 ] ).toHaveProperty( 'attrs', { key: 'value' } );
} );

test( 'innerBlocks is a list', () => {
expect( parse( 'freeform has empty innerBlocks' )[ 0 ] ).toHaveProperty( 'innerBlocks', [] );
expect( parse( '<!-- wp:void /-->' )[ 0 ] ).toHaveProperty( 'innerBlocks', [] );
expect( parse( '<!-- wp:block --><!-- /wp:block -->' )[ 0 ] ).toHaveProperty( 'innerBlocks', [] );

const withInner = parse( '<!-- wp:block --><!-- wp:inner /--><!-- /wp:block -->' )[ 0 ];
expect( withInner ).toHaveProperty( 'innerBlocks', expect.any( Array ) );
expect( withInner.innerBlocks ).toHaveLength( 1 );

const withTwoInner = parse( '<!-- wp:block -->a<!-- wp:first /-->b<!-- wp:second /-->c<!-- /wp:block -->' )[ 0 ];
expect( withTwoInner ).toHaveProperty( 'innerBlocks', expect.any( Array ) );
expect( withTwoInner.innerBlocks ).toHaveLength( 2 );
} );

test( 'innerHTML is a string', () => {
expect( parse( 'test' )[ 0 ] ).toHaveProperty( 'innerHTML', expect.any( String ) );
expect( parse( '<!-- wp:test /-->' )[ 0 ] ).toHaveProperty( 'innerHTML', expect.any( String ) );
expect( parse( '<!-- wp:test --><!-- /wp:test -->' )[ 0 ] ).toHaveProperty( 'innerHTML', expect.any( String ) );
expect( parse( '<!-- wp:test -->test<!-- /wp:test -->' )[ 0 ] ).toHaveProperty( 'innerHTML', expect.any( String ) );
} );
} );

Expand Down Expand Up @@ -131,7 +188,13 @@ export const phpTester = ( name, filename ) => makeTest(
}

try {
return JSON.parse( process.stdout );
/*
* Due to an issue with PHP's json_encode() serializing an empty associative array
* as an empty list `[]` we're manually replacing the already-encoded bit here.
*
* This is an issue with the test runner, not with the parser.
*/
return JSON.parse( process.stdout.replace( /"attrs":\s*\[\]/g, '"attrs":{}' ) );
} catch ( e ) {
throw new Error( 'failed to parse JSON:\n' + process.stdout );
}
Expand Down
Loading

0 comments on commit fdb8add

Please sign in to comment.