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

Markdown: Leave Gutenberg content intact #10635

Merged
merged 1 commit into from Nov 20, 2018

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Nov 15, 2018

This PR updates the Markdown module to leave any Gutenberg content untouched, in order to not mess up the block markup validation and parsing.

Fixes #10634

Changes proposed in this Pull Request:

  • Update the Markdown module to leave untouched any content that contains Gutenberg blocks.

Testing instructions:

  • Make sure you have the latest WordPress 5.0 beta.
  • Checkout this branch.
  • Make sure your site is connected.
  • Make sure Markdown is enabled.
  • Write a Gutenberg post with various blocks.
  • After saving and refreshing, verify there are no validation errors.
  • Try the Markdown block.
  • Verify it works well with various content.
  • Install and activate the Classic Editor plugin.
  • Write a post in the classic editor and verify Markdown still works well there.

Proposed changelog entry for your changes:

  • Make Markdown module compatible with the Gutenberg editor in WordPress 5.0.

@tyxla tyxla added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Markdown [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Pri] High [Status] Needs Testing We need to add this change to the testing call for this month's release labels Nov 15, 2018
@tyxla tyxla added this to the 6.8 milestone Nov 15, 2018
@tyxla tyxla self-assigned this Nov 15, 2018
@tyxla tyxla requested review from a team November 15, 2018 15:39
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D20897-code. (newly created revision)

@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: December 4, 2018.
Scheduled code freeze: November 27, 2018

Generated by 🚫 dangerJS

Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

I tested this on my personal site (which was impacted by this bug). With this PR, the text is not transformed (resulting in the paragraph breaks between blocks being left intact). :shipit:

@jeherve jeherve added [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Testing We need to add this change to the testing call for this month's release labels Nov 15, 2018
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

It works well on my end!

Copy link
Member

@mdawaffe mdawaffe left a comment

Choose a reason for hiding this comment

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

@dmsnell suggested updating the PHP Markdown code to run on Classic Blocks when the post was a Gutenberg post.

Dennis, is there a simple API for that these days?

Even if there is, I'm happy with the solution in this PR for now. I just flagged it as "Request Changes" so we could discuss it before merging.

Copy link
Member

@mdawaffe mdawaffe left a comment

Choose a reason for hiding this comment

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

An alternative solution has been proposed in D20852-code. Let's consolidate on one.

@brandonpayton
Copy link
Member

The alternate solution in D20852 is to allow markdown processing but stop the transform from mangling paragraphs. I think I may prefer the simplicity and clarity of this PR. Markdown processing could break blocks in others ways, and it would be good to avoid that.

@mdawaffe
Copy link
Member

mdawaffe commented Nov 15, 2018

What tests do we need to do for existing Markdown content? (That is, Markdown content that was published prior to Gutenberg/WordPress 5.0.)

Edit: I mean - what should we add to the Testing instructions for this PR or whatever eventual fix?

@brandonpayton
Copy link
Member

One other thing to note is that D20852 fixes an issue with the markdown plugin not being properly disabled when Gutenberg is enabled by a user. I'm planning to separate that fix into a separate patch and land that soon.

The issue we were troubleshooting was for mixed content that results when a user:

  1. Enables Gutenberg.
  2. Adds block content to an existing post with classic content.
  3. Disables Gutenberg.
  4. Edits the same post with the classic editor and finds their block content to be missing.

If we fix WP.com to properly disable the markdown transform when inserting posts from Gutenberg, the above issue is resolved.

But then we have another issue when editing mixed block/non-block in the classic editor:
Markdown processing removes p tags, but wpautop isn't called to re-add them because the post contains block content.

@brandonpayton
Copy link
Member

What tests do we need to do for existing Markdown content? (That is, Markdown content that was published prior to Gutenberg/WordPress 5.0.)

I'm not sure I understand but will add some comments in case they help.

When an existing markdown post is edited in Gutenberg, the transformed content is loaded by Gutenberg from post_content. This is because the easy-markdown plugin puts the original markdown content in post_content_filtered and the output of the markdown transform in post_content. When loading a post in the classic editor, easy-markdown uses the edit_post_content filter to provide the original markdown content, but this swap does not occur via the way Gutenberg loads posts. This means that editing a markdown post via Gutenberg uses the output of the markdown transform and not the original markdown.

@mdawaffe
Copy link
Member

mdawaffe commented Nov 15, 2018

When an existing markdown post is edited in Gutenberg, the transformed content is loaded by Gutenberg from post_content. This is because the easy-markdown plugin puts the original markdown content in post_content_filtered and the output of the markdown transform in post_content. When loading a post in the classic editor, easy-markdown uses the edit_post_content filter to provide the original markdown content, but this swap does not occur via the way Gutenberg loads posts. This means that editing a markdown post via Gutenberg uses the output of the markdown transform and not the original markdown.

Interesting, thanks for the insight. I'm not sure if that behavior makes backward compatibility easier or harder :)

I feel the most important thing is to preserve the content as viewed on the site. The behavior above seems to ensure that.

I feel it's less important to preserve the content as viewed in the editor. That is, if we Gutenbergify the for-edit-content, as long as it's "semantically" the same even though it's been syntactically changed is maybe OK?

But basically, I want us to be sure to test these scenarios (like the complicated one you outlined above) :)

@tyxla
Copy link
Member Author

tyxla commented Nov 16, 2018

Thanks for the alternate solution! I agree we'll need thorough testing in order to decide which solution works best and has as little negative impact as possible, on both old and new content. Perhaps @Automattic/flowpatrol could help with this?

@jeherve
Copy link
Member

jeherve commented Nov 20, 2018

I'll merge this for now, while leaving #10642 open so we can discuss and implement a fancier alternative in the future.

@jeherve jeherve dismissed mdawaffe’s stale review November 20, 2018 21:04

Discussing alternatives in a new issue

@jeherve jeherve merged commit 6474c03 into master Nov 20, 2018
@jeherve jeherve deleted the try/fix-markdown-gutenberg-wp-50-bug branch November 20, 2018 21:04
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 20, 2018
jeherve pushed a commit that referenced this pull request Nov 20, 2018
This PR updates the Markdown module to leave any Gutenberg content untouched, in order to not mess up the block markup validation and parsing.

Fixes #10634

#### Changes proposed in this Pull Request:

* Update the Markdown module to leave untouched any content that contains Gutenberg blocks.

#### Testing instructions:

* Make sure you have the latest WordPress 5.0 beta.
* Checkout this branch.
* Make sure your site is connected.
* Make sure Markdown is enabled.
* Write a Gutenberg post with various blocks.
* After saving and refreshing, verify there are no validation errors.
* Try the Markdown block.
* Verify it works well with various content.
* Install and activate the Classic Editor plugin.
* Write a post in the classic editor and verify Markdown still works well there.

#### Proposed changelog entry for your changes:

* Make Markdown module compatible with the Gutenberg editor in WordPress 5.0.
@jeherve
Copy link
Member

jeherve commented Nov 20, 2018

Cherry-picked to branch-6.8 in c395a4f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Markdown [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Pri] High Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants