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: Normalize data types and fix default implementation #10107

Merged
merged 5 commits into from Oct 6, 2018

Conversation

Projects
None yet
2 participants
@dmsnell
Contributor

dmsnell commented Sep 22, 2018

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.
@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Sep 24, 2018

Contributor

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.

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 self-assigned this Oct 4, 2018

dmsnell and others added some commits Sep 22, 2018

Parser: Normalize data types and fix default implementation
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

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Oct 5, 2018

Contributor

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?

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 approved these changes Oct 6, 2018

❤️

@dmsnell dmsnell merged commit 49b67d3 into master Oct 6, 2018

2 checks passed

codecov/project 49.31% (+<.01%) compared to 86dad5f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dmsnell dmsnell deleted the parser/normalize-data-types branch Oct 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment