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

Parser: Fix JS/PHP inconsistencies with empty attributes #11434

Merged
merged 3 commits into from
Nov 10, 2018

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Nov 2, 2018

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.

@dmsnell dmsnell added the [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f label Nov 2, 2018
@dmsnell dmsnell requested review from mcsf, pento and a team November 2, 2018 19:20
@dmsnell dmsnell force-pushed the parser/inconsistent-empty-attrs branch 2 times, most recently from 7ccba0b to 7613e65 Compare November 3, 2018 16:13
@dmsnell dmsnell changed the title WIP: Parser: Fix JS/PHP inconsistency in spec parser on empty attributes Parser: Fix JS/PHP inconsistency in spec parser on empty attributes Nov 8, 2018
dmsnell added a commit that referenced this pull request Nov 9, 2018
Since the introduction of the default parsers we have had a bug in the
PHP version whereby inner blocks were being popped onto the output
stack as PHP classes instead of as simple associated arrays. Blocks
that weren't inner blocks were being properly type-casted as arrays.

This patch adds the cast in where it's needed in order to fix this
inconsistent behavior.

So far this hasn't caused any troubles or exposed itself but while
working on other PRs like #11434 it became evident in test code.
@mcsf
Copy link
Contributor

mcsf commented Nov 9, 2018

I was about to approve this, but in the course of testing I found an issue that is also present in master:

Open Gutenberg's Code Editor, paste the following:

<!-- wp:foo {"id":824} -->
.
<!-- /wp:foo -->

<!-- wp:foo {} -->
.
<!-- /wp:foo -->

<!-- wp:foo -->
.
<!-- /wp:foo -->

I expected this to be preserved verbatim or at the very least to have the middle block "self-correct" by deleting its empty attributes object. Instead, upon blurring the code editor, I get this:

<!-- wp:foo {"id":824} -->
.
<!-- /wp:foo -->

<p><!-- wp:foo {} --><br>
.<br>
<!-- /wp:foo --></p>
<p><!-- wp:foo --><br>
.<br>
<!-- /wp:foo --></p>

Comparing the two spec and default parsers:

image

image


I assume this to be independent of the current PR, so it's up to you whether we should address it separately. In any case, 👍.

@mcsf
Copy link
Contributor

mcsf commented Nov 9, 2018

^ to be clear, I'm not concerned about the parser discrepancies when it comes to collecting empty freeform nodes — just the missing core/foo blocks. :)

@dmsnell
Copy link
Member Author

dmsnell commented Nov 9, 2018

Thanks for pointing out that bug @mcsf - I saw that a day or two ago and forgot to submit a new PR to fix it - #11690

I believe this is due to demanding that attributes aren't empty, but there's no reason they can't be.

@dmsnell dmsnell force-pushed the parser/inconsistent-empty-attrs branch from 84a57db to e71c1ee Compare November 9, 2018 19:00
@dmsnell
Copy link
Member Author

dmsnell commented Nov 9, 2018

Squashed and rebased; former HEAD was 052c689

@dmsnell dmsnell changed the title Parser: Fix JS/PHP inconsistency in spec parser on empty attributes Parser: Fix JS/PHP inconsistencies with empty attributes Nov 9, 2018
@dmsnell
Copy link
Member Author

dmsnell commented Nov 9, 2018

@mcsf I folded the {} fix into this PR since this is where all the tests are introduced. should be fixed for you now - thanks again for pointing that out.

@gwwar
Copy link
Contributor

gwwar commented Nov 10, 2018

With d1acdf6 @mcsf are we good to 🚢 ?

dmsnell added a commit that referenced this pull request Nov 10, 2018
Since the introduction of the default parsers we have had a bug in the
PHP version whereby inner blocks were being popped onto the output
stack as PHP classes instead of as simple associated arrays. Blocks
that weren't inner blocks were being properly type-casted as arrays.

This patch adds the cast in where it's needed in order to fix this
inconsistent behavior.

So far this hasn't caused any troubles or exposed itself but while
working on other PRs like #11434 it became evident in test code.
Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 Looks good!

@dmsnell dmsnell merged commit fdb8add into master Nov 10, 2018
@dmsnell dmsnell deleted the parser/inconsistent-empty-attrs branch November 10, 2018 02:13
@mtias mtias added this to the 4.3 milestone Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants