Add PHP parser based on PEG.js grammar #1152

nylen · 2017-06-13T01:13:34Z

There is a lot left to be done here, but this PR takes steps towards unifying our parsing logic across JS and PHP by using the same PEG grammar in both places.

There is a library for this that we can use: php-pegjs. It's a PEG.js plugin that generates a parser in PHP instead of JavaScript. However, there are lots of bits of JavaScript code interspersed in our PEG to make it work, and unfortunately, php-pegjs expects those to be written in PHP only.

So, this PR also points to my fork of php-pegjs which allows these code blocks to be specified in both PHP and JavaScript (see Nordth/php-pegjs#5).

Once finished, this will fix #1086 and hopefully also other issues such as #882.

Remaining items:

PHP 5.2 support: Remove namespaces in generated php-pegjs output; allow specifying a function prefix instead. Eliminate use of closures.
Actually use the generated parser in the plugin
Add PHPUnit tests for parser output using our existing HTML fixtures.
Require php-pegjs as a normal, stable dependency, by getting the needed changes merged into the main repo or a fork.

It would also be nice to upgrade php-pegjs to support newer versions of pegjs - while downgrading to pegjs 0.8.x causes the pegjs-loader peer dependency not to be met, in the actual code this will work fine.

dmsnell · 2017-06-13T05:57:31Z

Thanks @nylen!

This is something I had also planned on looping back around to. I'm thinking it may be possible to end up with a grammar that has little-to-no JS or PHP.

That shouldn't stop this from moving forward, but I think that we can be carefully plan out what the parser should be parsing and ignoring and automatically return the right data structures. For example, I was thinking of ripping out the keyValue function anyway if I can ignore the part of the parse we don't want.

westonruter · 2017-06-14T15:31:16Z

Awesome. I'm excited about this.

Another problem with the generated PHP parser in 5.2 is that it using closures.

westonruter · 2017-06-14T15:33:05Z

.travis.yml

      npm install || exit 1
      npm run build || exit 1
+      # Syntax check for php-pegjs parser
+      node bin/create-php-parser.js || exit 1


Should this be added to the NPM build and dev scripts?

I think not yet - instead, we need to make sure we run our existing parsing tests against the PHP parser as well.

aduth · 2017-06-14T16:25:36Z

blocks/api/post.pegjs

+  {
+    /** <?php
+    return array(
+      'blockType'  => $blockType,


Note: With #974, we'll need to update naming here to blockName

dmsnell · 2017-06-14T22:09:40Z

I spoke with @westonruter today about this and I'm really wondering if this is the right path. I'm pretty sure that the generated parser for PHP will be very sub-par on performance and the library doesn't appear to have been touched in over two years.

@nylen what do you think about simply having a separate parser file for PHP. Copying this for now and replacing the JS bits with PHP would seem reasonable, but I think that in the end we might be better served by a hand-written parser in PHP that implements the PEG spec simply on account of the different PHP semantics.

nylen · 2017-06-20T18:07:18Z

@nylen what do you think about simply having a separate parser file for PHP

I missed this conversation, but I'm not a fan of having separate parsing logic, as it guarantees that this critical piece of logic will continue to diverge between client and server. See #1190 and #1213 for recent examples, and note that we still can't or don't test the server-side parser for these cases or to nearly the same extent as we do the client-side parser.

I'm pretty sure that the generated parser for PHP will be very sub-par on performance

What are your specific concerns here? PEG parsers are supposed to be quite performant.

and the library doesn't appear to have been touched in over two years

This is true; it seems likely that we will have to fork it for our needs.

dmsnell · 2017-06-20T18:13:21Z

What are your specific concerns here? PEG parsers are supposed to be quite performant.

My concern is how the parser generator will generate the PHP and the overhead it brings with it. My fears should not be trusted until we know it's a problem. On the other hand, it's hard to establish a baseline. How many ms should it take to parse a document?

as it guarantees that this critical piece of logic will continue to diverge between client and server

I see your point though it's my hope that we will always end up with more than one. The spec parser which is clear to read and implementations that meet the spec but are faster. Already I've had some decisions to tradeoff readability against speed within the PEG

nylen · 2017-06-23T12:54:48Z

I've rebased and updated this PR to use phpegjs, my fork of php-pegjs. It has WIP support for PEG.js 0.10.0 (the latest version, which we have been using in this repository).

There were some significant changes required to support the new version of PEG.js, and there are still some bugs, as evidenced by the failing PHP parser tests (newly introduced in this PR).

However, this is good progress all the same, and I still think this task is doable.

nylen · 2017-06-27T17:29:41Z

One step closer: the generated PHP parser now has a bunch of tests based on our existing fixtures. They pass everywhere except PHP 5.2. 🎉

The logic here has also been rebased to support JSON-style attributes (#1213).

Still left to do: PHP 5.2 support; actually use the new parser for do_blocks server-side parsing and rendering.

paulwilde · 2017-06-28T00:40:20Z

Honest question - Why should Gutenberg support a 11 year old version of PHP? Surely by the time Gutenberg is merged into core (Late 2017/Early 2018) it's time to think about updating the minimum requirements for PHP.

Gutenberg is a nice forward step in making WordPress a modern piece of software, so why should the PHP side of things suffer?

If Gutenberg is going to break backwards-compatibility, surely it's a good time to also force people to move away from a highly outdated and insecure version of PHP.

nylen · 2017-06-28T14:17:29Z

Honest question - Why should Gutenberg support a 11 year old version of PHP?

It would be nice if we didn't have to support PHP 5.2. However, I think this decision should be made along with the rest of core, and there are ongoing discussions about this.

Quoting from recent discussion in the #core-php Slack channel:

There will be a summary posted from the summit discussion within the next week or so (notes are being collected from the note takers currently). The consensus was to do as much as we can to encourage upgrading to see if we can get the percentage of users on old PHP versions down, and revisit in a little while. #41191 and #40934 were the first steps. Would be great to get further feedback on that post once it happens.

As far as supporting PHP 5.2 in this PR, the work is already mostly done, via this commit.

nylen · 2017-06-29T17:23:12Z

This is ready for review/merge.

Add `php-pegjs` and downgrade `pegjs` version accordingly Add script to generate PHP parser Update PEG grammar with PHP and JS code Run syntax check against php-pegjs parser Exclude generated PHP parser from phpcs checks Fix lint errors

Or using the following commands (TODO - add a npm script for this?): ``` bin/create-php-parser.js RUN_PARSER_TESTS=1 phpunit ```

Fix all the things (except PHP 5.2 support).

Otherwise PHPUnit 4.5.0 (used with PHP 5.6 Travis build) complains about a suite with no tests in it.

Adds support for generating a parser compatible with PHP 5.2.

This will simplify the process of loading the plugin. Otherwise we'd have to add this step to `npm run dev` and a couple of other places.

This reverts commit 57389f49de1bce57bf97012df4d207bf2fffdea3.

nylen · 2017-06-30T03:04:54Z

Merging to get this and the breaking change layered on top of it (#1593) into a release as soon as possible.

aduth · 2017-07-03T14:22:18Z

These changes define pegjs as a duplicate dependency between dependencies and devDependencies, with a warning on npm install:

npm WARN The package pegjs is included as both a dev and production dependency.

nylen · 2017-07-05T09:49:58Z

These changes define pegjs as a duplicate dependency between dependencies and devDependencies, with a warning on npm install

Fixed in #1708.

nylen requested review from dmsnell and westonruter June 13, 2017 01:13

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

nylen mentioned this pull request Jun 14, 2017

Support specifying both PHP and JS code in code blocks Nordth/php-pegjs#5

Closed

westonruter reviewed Jun 14, 2017

View reviewed changes

aduth reviewed Jun 14, 2017

View reviewed changes

This was referenced Jun 15, 2017

Update PHP “parser” to recognize shorthand block syntax #1190

Merged

Fix parsing in do_blocks() and rendering of blocks on frontend in the_content #1244

Merged

nylen force-pushed the add/php-pegjs branch from 70a3181 to 5e92471 Compare June 23, 2017 12:50

nylen force-pushed the add/php-pegjs branch from 5e92471 to 203f64c Compare June 27, 2017 17:11

nylen force-pushed the add/php-pegjs branch 5 times, most recently from 57389f4 to 83fb025 Compare June 29, 2017 17:11

nylen removed the [Status] In Progress Tracking issues with work in progress label Jun 29, 2017

nylen mentioned this pull request Jun 29, 2017

Improve consistency of block names #1593

Merged

Introduce php-pegjs

753aab9

Add `php-pegjs` and downgrade `pegjs` version accordingly Add script to generate PHP parser Update PEG grammar with PHP and JS code Run syntax check against php-pegjs parser Exclude generated PHP parser from phpcs checks Fix lint errors

nylen added 17 commits June 30, 2017 04:57

Change php-pegjs to phpegjs (forked version)

2533fc8

Add parser output to full-content test fixtures

01e661f

Run tests against PHP parser during Travis CI builds

b6337f7

Or using the following commands (TODO - add a npm script for this?): ``` bin/create-php-parser.js RUN_PARSER_TESTS=1 phpunit ```

Update parser fixture files after rebase

c88b9f4

Decode block JSON attributes as PHP arrays, not StdClass objects

21c2ac3

Remove error logging and clarify variable name

15b9966

Upgrade phpegjs to 1.0.0-beta2

e29c413

Fix all the things (except PHP 5.2 support).

Use a different method of skipping the PHP parser tests

8eb22a7

Otherwise PHPUnit 4.5.0 (used with PHP 5.6 Travis build) complains about a suite with no tests in it.

Make phpcs happy again

8f8d649

Upgrade phpegjs to 1.0.0-beta3

dcb968a

Adds support for generating a parser compatible with PHP 5.2.

Do not use namespaces in generated PHP parser

dd0f205

Add generated PHP parser to repository

eff5ee4

This will simplify the process of loading the plugin. Otherwise we'd have to add this step to `npm run dev` and a couple of other places.

During Travis build, make sure PHP parser is up to date

4c4fe43

Fix a few test fixtures that needed to change after rebase

b950437

Modify the PHP parser (this should fail the build)

ef05ef1

Revert "Modify the PHP parser (this should fail the build)"

75d418f

This reverts commit 57389f49de1bce57bf97012df4d207bf2fffdea3.

Use PHP PEG parser in plugin

f095297

nylen force-pushed the add/php-pegjs branch from 1b87687 to f095297 Compare June 30, 2017 02:59

nylen merged commit 3c8ea7e into master Jun 30, 2017

nylen deleted the add/php-pegjs branch June 30, 2017 03:05

This was referenced Jun 30, 2017

The server-side rendering block matcher doesn't match blocks with - (hyphen) #882

Closed

Remove RUN_PARSER_TESTS flag from PHPUnit tests #1605

Merged

youknowriad mentioned this pull request Jul 3, 2017

Strip out comments when rendering "the_content" #1494

Closed

nylen mentioned this pull request Jul 3, 2017

PEG.js based parser introduced a performance regression #1646

Closed

swissspidy mentioned this pull request Jul 4, 2017

Clean up PEG.js package references #1708

Merged

nylen mentioned this pull request Jul 7, 2017

Improve PHP parsing performance #1775

Merged

chrisvanpatten mentioned this pull request May 10, 2020

Bring the PHP implementation of the block parser inline with WordPress coding standards #21891

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PHP parser based on PEG.js grammar #1152

Add PHP parser based on PEG.js grammar #1152

nylen commented Jun 13, 2017 •

edited

Loading

dmsnell commented Jun 13, 2017

westonruter commented Jun 14, 2017

westonruter Jun 14, 2017

nylen Jun 20, 2017

aduth Jun 14, 2017

dmsnell commented Jun 14, 2017

nylen commented Jun 20, 2017

dmsnell commented Jun 20, 2017

nylen commented Jun 23, 2017

nylen commented Jun 27, 2017 •

edited

Loading

paulwilde commented Jun 28, 2017 •

edited

Loading

nylen commented Jun 28, 2017 •

edited

Loading

nylen commented Jun 29, 2017

nylen commented Jun 30, 2017

aduth commented Jul 3, 2017

nylen commented Jul 5, 2017

Add PHP parser based on PEG.js grammar #1152

Add PHP parser based on PEG.js grammar #1152

Conversation

nylen commented Jun 13, 2017 • edited Loading

dmsnell commented Jun 13, 2017

westonruter commented Jun 14, 2017

westonruter Jun 14, 2017

Choose a reason for hiding this comment

nylen Jun 20, 2017

Choose a reason for hiding this comment

aduth Jun 14, 2017

Choose a reason for hiding this comment

dmsnell commented Jun 14, 2017

nylen commented Jun 20, 2017

dmsnell commented Jun 20, 2017

nylen commented Jun 23, 2017

nylen commented Jun 27, 2017 • edited Loading

paulwilde commented Jun 28, 2017 • edited Loading

nylen commented Jun 28, 2017 • edited Loading

nylen commented Jun 29, 2017

nylen commented Jun 30, 2017

aduth commented Jul 3, 2017

nylen commented Jul 5, 2017

nylen commented Jun 13, 2017 •

edited

Loading

nylen commented Jun 27, 2017 •

edited

Loading

paulwilde commented Jun 28, 2017 •

edited

Loading

nylen commented Jun 28, 2017 •

edited

Loading