Skip to content

Parser: Normalize data types and fix default implementation#10107

Merged
dmsnell merged 5 commits intomasterfrom
parser/normalize-data-types
Oct 6, 2018
Merged

Parser: Normalize data types and fix default implementation#10107
dmsnell merged 5 commits intomasterfrom
parser/normalize-data-types

Conversation

@dmsnell
Copy link
Copy Markdown
Member

@dmsnell dmsnell commented Sep 22, 2018 •
edited
Loading

Resolves #10041
Resolves #10047

A few inconsistencies have remained in the grammar specification
concerning freeform blocks and blocks without attributes in the
block delimiters. Freeform blocks were returned without block
names and blocks without attributes returned null instead of
an empty set of attributes.

Further, the default parser implementation (from #8083) was
returning an array of block objects instead of an array of
generic arrays. This resulted in mismatches in PHP of accessing
properties with $block[ 'attrs' ] syntax vs $block->attrs
syntax.

In this patch I've updatd the specification to remove all of
the type ambiguity and have updated the default parser to match
it. After this patch every block should be accessible as a normal
array in PHP and have all properties: blockName, attrs,
innerBlocks, and innerHTML. If no attributes are specified
then attrs will be an empty set (in JavaScript {} and in
PHP array()).

Status

I haven't tested this yet. I'd like some feedback on the design change
though. My plan is to run the tests through the parser comparator and
make sure that the spec and the default parsers remain consistent.

If you look at the automated test results you can see what the impact is
of making HTML soup turn implicitly into a freeform block. I'd appreciate
some feedback on what you think of that. Personally I think it's a reasonable
change: at the time we didn't do that we weren't as sure of what would
happen with the "HTML soup" but it seems like we've locked down the idea
of the classic block which is core/freeform - maybe I'm wrong 🤷‍♂️

Checklist:

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

@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 Sep 22, 2018
@dmsnell dmsnell requested review from aduth, mcsf and pento September 22, 2018 02:19
@dmsnell dmsnell force-pushed the parser/normalize-data-types branch from cab05fe to 6a48131 Compare September 22, 2018 02:20
@dmsnell dmsnell mentioned this pull request Sep 22, 2018
Closed
@dmsnell dmsnell force-pushed the parser/normalize-data-types branch from 6a48131 to 8800237 Compare September 22, 2018 02:23
@dmsnell dmsnell added this to the 4.0 milestone Sep 22, 2018
@dmsnell dmsnell force-pushed the parser/normalize-data-types branch from 8800237 to 335f5b9 Compare September 22, 2018 02:29
@dmsnell dmsnell added the [Feature] Block API API that allows to express the block paradigm. label Sep 22, 2018
@mcsf
Copy link
Copy Markdown
Contributor

mcsf commented Sep 24, 2018

Thanks for working on this. My thoughts:

  • Making sure blocks are represented as plain arrays is imperative. Plus, it's an actual discrepancy with the spec parser.
  • The other consistency changes, namely with attrs and innerBlocks, make sense, but I'm a bit on the fence because they require updating the spec so late in the development cycle. We can get them in, though I wouldn't be surprised if it broke something for anyone already working around these inconsistencies in their own projects. Perhaps an action question is: can we get some deprecation mechanism for this? Since outputting PHP warnings can break websites that aren't properly configured, it could be as simple as blindly printing a deprecation notice in administrator's console with every page load.
  • Adopting the 'blockName' => 'core/freeform' is what I'm most concerned about. It makes some sense — and I would've probably advocated for this a year ago, as it more closely matches the user's experience with the Classic block — though now it might be a little late.* On the other hand, freeform is special in that one is not supposed to have two contiguous "freeform blocks"; instead, freeform represents a catch-all of anything that isn't a block, and this may be a strong enough reason to keep it as a nameless object.

*: By late I don't mean that we can't ever circle back to this, but rather that at this stage when we're so close to 5.0 I'd really rather focus on other pieces, and we could have another look at this in a post-5.0 cycle.

@mcsf mcsf force-pushed the parser/normalize-data-types branch from 335f5b9 to 783adcb Compare October 2, 2018 01:26
@mcsf mcsf self-assigned this Oct 4, 2018
dmsnell and others added 3 commits October 4, 2018 19:56
@dmsnell @mcsf
Parser: Normalize data types and fix default implementation
bf17aab 
Resolves #10041
Resolves #10047

A few inconsistencies have remained in the grammar specification
concerning freeform blocks and blocks without attributes in the
block delimiters. Freeform blocks were returned without block
names and blocks without attributes returned `null` instead of
an empty set of attributes.

Further, the default parser implementation (from #8083) was
returning an array of block objects instead of an array of
generic arrays. This resulted in mismatches in PHP of accessing
properties with `$block[ 'attrs' ]` syntax vs `$block->attrs`
syntax.

In this patch I've updatd the specification to remove all of
the type ambiguity and have updated the default parser to match
it. After this patch every block should be accessible as a normal
array in PHP and have all properties: `blockName`, `attrs`,
`innerBlocks`, and `innerHTML`. If no attributes are specified
then `attrs` will be an empty set (in JavaScript `{}` and in
PHP `array()`).
@mcsf
Don't assign block name to freeform units
0b778b6
@mcsf
Update fixtures for integration tests
1b686b6
@mcsf mcsf force-pushed the parser/normalize-data-types branch from c2cc5bc to 1b686b6 Compare October 5, 2018 00:46
@mcsf
Copy link
Copy Markdown
Contributor

mcsf commented Oct 5, 2018

I haven't tested this yet. I'd like some feedback on the design change though. My plan is to run the tests through the parser comparator and make sure that the spec and the default parsers remain consistent.

@dmsnell, I've shrunk the scope of this to exclude the introduction of core/freeform as a blockName and adjusted the tests accordingly. Could you have a go with the comparator?

@mcsf mcsf force-pushed the parser/normalize-data-types branch from b18e94e to 25d31ca Compare October 6, 2018 05:27
mcsf
mcsf approved these changes Oct 6, 2018
View reviewed changes
Copy link
Copy Markdown
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.

❤️

@dmsnell dmsnell merged commit 49b67d3 into master Oct 6, 2018
@dmsnell dmsnell deleted the parser/normalize-data-types branch October 6, 2018 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcsf mcsf mcsf approved these changes

@pento pento Awaiting requested review from pento

@aduth aduth Awaiting requested review from aduth

Assignees

@mcsf mcsf

Labels

[Feature] Block API API that allows to express the block paradigm. [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f

Projects

None yet

Milestone

4.0

Development

Successfully merging this pull request may close these issues.

2 participants

@dmsnell @mcsf