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

Fix: Empty Classic Editor inside innerBlock fatal error #17164

Conversation

@donmhico
Copy link
Contributor

commented Aug 23, 2019

Description

The problem is that the innerBlocks array gets undefined element if the content parsed is <!-- wp:freeform /-->. So the change just removes any undefined element in the innerBlocks. The merit of this solution is that any previously saved posts with this case will work.

How has this been tested?

  1. Followed the steps provided in #17138.
  2. Confirmed that the bug exist.
  3. Applied the fix.
  4. Re-do step 1.
  5. Failed to reproduce the bug.

Types of changes

Fixes #17138.

Checklist:

  • My code is tested.
@donmhico donmhico changed the title Fix: Empty Classic Editor insider innerBlock fatal error Fix: Empty Classic Editor inside innerBlock fatal error Aug 23, 2019
@youknowriad youknowriad requested a review from mcsf Aug 23, 2019
@mcsf

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

Thanks for working on this! I have my doubts regarding the fix itself. Indeed, the parser (createBlockWithFallback) is automatically discarding Freeform blocks (which by default are Classic) that are empty, and I'm not sure why this should happen (this was added over two years ago):

// Include in set only if type was determined.
if ( ! blockType || ( ! innerHTML && isFallbackBlock ) ) {
return;
}

The result of the above is that the innerBlocks of a block containing an empty Classic block will have a block that is merely undefined. The present PR works around this by dropping any such undefined block out of innerBlocks, but a Classic block is still a block to be handled.

@youknowriad, in response to your comment at #663 (comment), do you think this here is a valid case for changing the parser's condition?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

@mcsf It seems like a valid case yes. I think we should just make sure that when we serialize a post composed of just empty classic blocks, we shouldn't end up with a lot of useless empty lines and just trim them from the serialization. (So basically move the fix to the serialization)

@mcsf

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

@mcsf It seems like a valid case yes. I think we should just make sure that when we serialize a post composed of just empty classic blocks, we shouldn't end up with a lot of useless empty lines and just trim them from the serialization. (So basically move the fix to the serialization)

That makes sense. :)

@donmhico, is this something you’d like to take on? Would you also be able to add tests to cover this issue? Otherwise we can split the load.

@donmhico

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

@youknowriad @mcsf - Thanks for the feedback. I'm happy to take this on. However, I may need more guidance as I'm just a new contributor to Gutenberg.

First is, I'm unsure if the fix I provided is still valid and if I should just create unit tests for it. Or do I have to look into the serialization you both mentioned?

@mcsf

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

I'm happy to take this on. However, I may need more guidance as I'm just a new contributor to Gutenberg.

Thanks! We'll guide you, for sure.

First is, I'm unsure if the fix I provided is still valid and if I should just create unit tests for it. Or do I have to look into the serialization you both mentioned?

So this particular PR would have to change so that:

  • in the parser, instead of filtering out undefined inner blocks, we make sure that empty Classic/Freeform blocks are fully parsed
  • in the serialiser, we make sure that we do discard empty areas of content
@donmhico

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

Thanks @mcsf. I'll try to do the changes tomorrow. I'll ping you if I have any inquiries.

@donmhico

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

Update @mcsf @youknowriad.

in the parser, instead of filtering out undefined inner blocks, we make sure that empty Classic/Freeform blocks are fully parsed

From what I can see, this can be fixed by doing removing freeformContentFallbackBlock as a check for isFallbackBlock from

const isFallbackBlock = (
name === freeformContentFallbackBlock ||
name === unregisteredFallbackBlock
);

to

    const isFallbackBlock = (
-       name === freeformContentFallbackBlock ||
        name === unregisteredFallbackBlock
    );

However, like you've said @mcsf. I'm unsure why it freeformContentFallbackBlock is included in the check in the first place.

Also applying the change above and running npm test produces errors. How should I proceed with this?

in the serialiser, we make sure that we do discard empty areas of content

I'm unsure how to achieve this. Any guidance would be greatly appreciated.

@mcsf

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

Hi, @donmhico. Riad is away for a few days, but I'm around to assist.

From what I can see, this can be fixed by doing removing freeformContentFallbackBlock as a check for isFallbackBlock […]

Yeah, this would achieve it.

However, like you've said @mcsf. I'm unsure why it freeformContentFallbackBlock is included in the check in the first place.

Let's assume that the only reason was to avoid piling up "empty" freeform blocks that would later be serialised into unnecessary whitespace. See this particular comment: #663 (comment)

in the serialiser, we make sure that we do discard empty areas of content

I'm unsure how to achieve this. Any guidance would be greatly appreciated.

So, in the same way that we were checking for something like isFreeform && ! innerHTML in the parser, we would reverse the logic and do that sort of checking somewhere in the serialiser's call tree:

export function serializeBlock( block, { isInnerBlocks = false } = {} ) {
const blockName = block.name;
const saveContent = getBlockContent( block );
if (
( blockName === getUnregisteredTypeHandlerName() ) ||
( ! isInnerBlocks && blockName === getFreeformContentHandlerName() )
) {
return saveContent;
}
const blockType = getBlockType( blockName );
const saveAttributes = getCommentAttributes( blockType, block.attributes );
return getCommentDelimitedContent( blockName, saveAttributes, saveContent );
}

Also applying the change above and running npm test produces errors. How should I proceed with this?

These are expected. In the tests, there is a particular suite that tests parsing and serialisation, and it should fail unless the tests are patched:

FAIL  test/integration/full-content/full-content.test.js (27.472s)

What this suite does is test, for all core block types, that a particular block output in parsed in a specific way, and that it later gets reserialised in another particular way. See full-content.test.js and all the test fixtures.

The tests now fail because the fixtures have a lot of whitespace in between blocks that now are recognised in the parser as empty freeform blocks, and these don't correspond with the expect result stated in the test fixtures.

One way to address this would be to patch all the fixtures. But, now that I'm thinking about this, I think we can approach the problem slightly differently:


You see, initially a Freeform/Classic block was defined in the parser as "any area of content that wasn't part of a proper block". This worked well conceptually and functionally until we introduced nesting. We eventually had to change this in #16477.

Nowadays, the serialiser will check to see whether a Freeform block is at the root of the content or whether it's nested in other blocks.

  • If it's at the root, it behaves just as before, and just outputs the Freeform content with no other wrapping.
  • If it's nested, then the serialiser wraps Freeform content as if it were a proper block:
<!-- wp:freeform -->
Some classic content
<!-- /wp:freeform -->

This brings me to the current PR. Here is what we could do in the parser:

  • If we run into a non-demarcated Freeform block that is not empty, parse it as you would normally.
  • If we run into a non-demarcated Freeform block that is empty, discard it.
  • If we run into a demarcated Freeform block (i.e. with the wp:freeform boundary), then parse it regardless of whether it's empty or not.

Since Freeform blocks in nested contexts are always demarcated, this would fix the parent issue.

The way that we can recognise whether we're dealing with a non-demarcated Freeform block is by looking at the block node's originalName in createBlockWithFallback, in which case it will be undefined. See for yourself: while in Gutenberg, pull up the console and evaluate the following:

wp.blockSerializationDefaultParser.parse(
`<!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:freeform /--></div></div>
<!-- /wp:group -->

<p>B</p>

<!-- wp:paragraph -->
<p>C</p>
<!-- /wp:paragraph -->`
)

the resulting array will show that the Freeform block with content B will have no name, while the empty Freeform block inside the Group block will indeed have a name.


This should be enough to get you started, @donmhico! If not, feel free to reach out!

@mcsf

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@donmhico, actually I'd like to merge more or less what you have now on this branch just so that we can release Gutenberg 6.4 with no critical errors in it. This would be a temporary fix — in which we still lose nested empty Classic blocks but we don't crash the application. I realise it's late in the day for you, so I think I'll merge your PR in a minute and we can continue refining this separately. :)

@mcsf
mcsf approved these changes Aug 26, 2019
@mcsf mcsf merged commit ef82afd into WordPress:master Aug 26, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@mcsf

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

Merged. Thanks for the patch, @donmhico!

donmhico added a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
)

* Fix: Empty Classic Editor insider innerBlock fatal error

* Expand inline comment
dratwas added a commit to callstack/gutenberg that referenced this pull request Aug 28, 2019
)

* Fix: Empty Classic Editor insider innerBlock fatal error

* Expand inline comment
gziolo added a commit that referenced this pull request Aug 29, 2019
* Fix: Empty Classic Editor insider innerBlock fatal error

* Expand inline comment
gziolo added a commit that referenced this pull request Aug 29, 2019
* Fix: Empty Classic Editor insider innerBlock fatal error

* Expand inline comment
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
)

* Fix: Empty Classic Editor insider innerBlock fatal error

* Expand inline comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.