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: Allow empty attributes in default parsers #11690

Closed
wants to merge 1 commit into from

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Nov 9, 2018

Since the introduction of the default parser in #8083 we have had a
subtle bug in the parsing which failed when empty attributes were
specified in a block's comment delimiter - {}

The absense of attributes was fine but empty attributes were a
failure. This is due to using +? in the RegExp tokenizer instead of
using *? (which allows for no inner content in the JSON string).

This patch updates the quantifier to restore functionality and fix the
bug. This didn't appear in practice because we don't intentionally set
{} as the attributes - the serializer drops it altogther, and our
tests didn't catch it for similar reasons.

Testing

Open up a post with the code editor.
Enter <!-- wp:test {} /--> and see if it breaks.
If it breaks, once you hit Save Draft it will replace it with <p><!-- wp:test {} /--></p>

If it's working, you won't see it change on Save Draft

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Since the introduction of the default parser in #8083 we have had a
subtle bug in the parsing which failed when empty attributes were
specified in a block's comment delimiter - `{}`

The absense of attributes was fine but _empty_ attributes were a
failure. This is due to using `+?` in the RegExp tokenizer instead of
using `*?` (which allows for no inner content in the JSON string).

This patch updates the quantifier to restore functionality and fix the
bug. This didn't appear in practice because we don't intentionally set
`{}` as the attributes - the serializer drops it altogther, and our
tests didn't catch it for similar reasons.
@dmsnell dmsnell added [Type] Bug An existing feature does not function as intended [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f labels Nov 9, 2018
@dmsnell dmsnell requested review from mcsf, pento and aduth November 9, 2018 18:39
Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix! Still, even though our serialiser drops the empty object, can we add a test case?

@dmsnell
Copy link
Member Author

dmsnell commented Nov 9, 2018

@mcsf the test is in #11434 and it'd be much easier from a merge-standpoint to push this fix out independently of the tests. I can go through all the work of moving around code and managing the conflicts if it's important to you, but I'd rather see us merge both this and that one right next to each other - this is a tangent that could have been rolled into #11434 but I wanted to keep it separate for the sake of separating different changes.

if you want the test then maybe I'll close this PR and add the fix in #11434

what do you say?

@dmsnell
Copy link
Member Author

dmsnell commented Nov 9, 2018

Except now I'm confused why the tests in #11434 are passing since this is tested there…will do some more investigation

@dmsnell
Copy link
Member Author

dmsnell commented Nov 9, 2018

now I'm confused why the tests in #11434 are passing

haha, it was passing because <p><!-- wp:void {} /--></p> is a freeform block whose attributes are {}

@dmsnell
Copy link
Member Author

dmsnell commented Nov 9, 2018

Merging this fix into #11434

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 [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants