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

Editor: Barnsbury page layout breaks editor in sandboxed site with Gutenberg 8.7 #44783

Closed
p-jackson opened this issue Aug 10, 2020 · 8 comments
Assignees

Comments

@p-jackson
Copy link
Member

Steps to reproduce

  1. Sandbox a test site (sandbox should be up-to-date enough that it has GB 8.7)
  2. Create a new page
  3. Choose the Barnsbury page layout (the vegetable one in the Home Pages category)
  4. Publish the page (not necessary to reproduce, we just want to see what happens on the frontend too)
  5. Refresh the page

What I expected

The editor reloads as expected. If you view the newly published page on front end you see the Barnsbury page layout. This is what happens when the site isn't sandboxed, so users aren't seeing the issue reported here.

What happened instead

After refreshing the editor placeholder appears for a bit but then the screen goes blank and fails to load.

If you load the page on the front end the page is filled with DOMDocument::loadHTML(): Tag time invalid in Entity warnings. I wonder if it's the editor trying to

Browser / OS version

Using GB 8.7. I believe this is the culprit because when I downgrade to 8.5.1 in the sandbox the warnings go away.

Screenshot / Video

Screenshot of the front end
Untitled

@p-jackson p-jackson changed the title Barnsbury page layout breaks editor in sandboxed site with Gutenberg 8.7 Editor: Barnsbury page layout breaks editor in sandboxed site with Gutenberg 8.7 Aug 10, 2020
@andrewserong
Copy link
Member

@ramonjd and I encountered this issue using the Stratford page layout, with the latest posts block set to display post date. If we access the site from the front-end, we get the same error:

image

And in the backend, it results in a white page and the editor won't load receiveEntityRecords throws the following error:

image

I think this would be because the PHP warning is being outputted to the response from the server. Do we know why these warnings are being outputted in the sandbox instead of being suppressed or being sent to error logs instead of being part of the response?

This error that we could replicate is only happening for us while sandboxed.

@andrewserong
Copy link
Member

Okay, my current hunch is that we're seeing this warning now due to the change in WordPress/gutenberg#24351 where gutenberg_apply_block_supports is now applied to these server rendered blocks (and in turn then calls DOMDocument::loadHTML(). It looks like previously the gutenberg_experimental_apply_classnames_and_styles function might have bailed earlier before than after that PR landed (I can see that previously it'd return $block_content if there was an empty $block_type->supports, so I wonder if that's a clue).

At any rate, in https://github.com/WordPress/gutenberg/blob/master/lib/block-supports/index.php#L54 DOMDocument::loadHTML() is suppressed using the @ error control operator, but I'm wondering if it'd work better to use libxml_use_internal_errors for this kind of suppression so that debug-like environments don't get this warning in the output? (That said, I'm not too sure how either approach here works internally)

@Addison-Stavlo I see you reviewed WordPress/gutenberg#24351, so I wondered if you might have more context here? CC: @ramonjd

@ramonjd
Copy link
Member

ramonjd commented Aug 11, 2020

is suppressed using the @ error control operator, but I'm wondering if it'd work better to use libxml_use_internal_errors

Thanks for getting to the bottom of it @andrewserong

I don't agree with using @ in general. It's convenient to suppress annoying errors, but it will also suppress errors that we probably don't expect and want to know about.

But DOMDocument does not support HTML5 so we're going to see more of this in development environments if we don't turn it off fully.

Therefore +1 on using libxml_use_internal_errors either to disable or enable user error handling.

@ramonjd
Copy link
Member

ramonjd commented Aug 11, 2020

Do we know why these warnings are being outputted in the sandbox instead of being suppressed or being sent to error logs instead of being part of the response?

We're setting our own error handler via set_error_handler in the config See https://stackoverflow.com/a/6621834

🤔 Wonder if that's it?

Confirmed. Commenting out our error handler allows @ to suppress the errors.

See 26096-pb for where we call set_error_handler

@andrewserong andrewserong self-assigned this Aug 11, 2020
andrewserong referenced this issue in lsl/gutenberg Aug 19, 2020
Sets the charset of the html passed into DomDocument to utf-8.

Replaces the mb_convert_encoding call replacing utf-8 with html entities
before handing off to DomDocument.

This avoids the need to later convert back to utf-8 from html entities
afterward. This secondary mb_convert_encoding call was converting not
only the utf-8 we converted earlier but also entity encoding html stored
inside data-* or other attributes of html elements.

Fixes Automattic/wp-calypso#44897
Maintains the fix for WordPress#24445 (WordPress#24447)
@lsl
Copy link
Contributor

lsl commented Aug 20, 2020

@andrewserong this wasn't fixed in WordPress/gutenberg/pull/24645, creating a new PR for this as I think it should probably be changed.

@andrewserong
Copy link
Member

Thanks so much @lsl!

@lsl
Copy link
Contributor

lsl commented Aug 20, 2020

WordPress/gutenberg/pull/24677

@lsl
Copy link
Contributor

lsl commented Sep 2, 2020

This should be fixed in 8.9 - WordPress/gutenberg@33c83fa

@lsl lsl closed this as completed Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants