Feature: Markdown support as a Jetpack Markdown-specific Gutenberg Block

[nuzzio]

Description

This PR will contain any supporting code for the Automattic/Jetpack PR Feature: Markdown support as a Markdown-specific Gutenberg Block.

My Goal is to minimize this PR to one change, and move all functionality to Jetpack.

Issues

• Move "Read Markdown for Editing Post" functionality to Jetpack.

How has this been tested?

Screenshots

Types of changes

There are two changes:

1. Breaking change: re-enable Markdown functionality in Jetpack and let Jetpack preserve non-markdown Gutenberg blocks here: Automattic/jetpack@a1c6611

2. Breaking change: Naive implementation which does some "filtering" the result of the REST dispatch request, in order to serve the Markdown content to Gutenberg on New/Edit post. This change should be moved to Jetpack as a proper filter triggered by an event of the dispatch flow of the WP_REST_Server. This has been changed to apply filters on the after_gutenberg_gets_post_to_edit event 36cd43c

Checklist:

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

[nuzzio]

Corresponding PR Automattic/jetpack#9357

[nuzzio]

If anyone has a little time, I am working on a Markdown block in Jetpack and in this PR I am working on making sure that the Markdown content gets returned to Gutenberg on Edit post.

In the classic implementation there is a filter which is run upon getting the post for editing. The filter swaps the post_content column contents for the post_content_filtered column, which is where unparsed Markdown is stored.

I made a naive implementation here: 737f1f9

Which works as intended but I think it would be a better practice to implement this functionality as a proper filter in Jetpack which is triggered by an event of the dispatch flow of the WP_REST_Server.

Which event would be the recommended one?

This has been changed to apply filters on the after_gutenberg_gets_post_to_edit event 36cd43c

[ehg]

What is the permanent fix here?

[nuzzio]

I would say the permanent fix is to remove this file if we determine that the fix I am proposing solves the requirements.

Disabling Markdown relies on this check gutenberg_content_has_blocks( $content ) which in turn is a simple return false !== strpos( $content, '<!-- wp:' );

The reason why it was implemented seems to be:

WPCOM markdown support causes issues when saving a Gutenberg post by stripping out the <p> tags. This adds a filter prior to saving the post via REST API to disable markdown support. Fixes markdown support provided by plugins Jetpack, JP-Markdown, and WP Editor.

My solution was to implement that Gutenberg block protection in WPCom_Markdown by relying on the existing jetpack_markdown_preserve_pattern functionality which I believe was intended for this purpose: https://github.com/Automattic/jetpack/blob/master/_inc/lib/markdown/gfm.php#L106

I preserve all non Gutenberg non-Markdown blocks here:

Automattic/jetpack@a1c6611#diff-c8a16334ab363043643dab428095f818R580

I do an equivalent of gutenberg_content_has_blocks( $content ) to decide whether to protect the content or not.

I may need some feedback from @mkaz and/or @aduth on the viability of this solution.

I don't know the exact nature of the JP-Markdown, and WP Editor conflicts that are mentioned but I think this solution may scale to address those conflicts by simply adding more Markdown exclusion patterns like: exclude anything outside of the Markdown block. Perhaps that could be included now.

[mkaz]

JP-Markdown and WP Editor use the same Jetpack markdown code, so any changes made would take time to filter down to those plugins, assuming they are still tracking Jetpack markdown.

Your approach seems like it would work, by not modifying Gutenberg posts in Jetpack's markdown. Though I have not tested it since its still in Jetpack development. Is there a target release for that or is it still in the works?

I think this would need to be done in multiple stages, since we can't remove the filter until the corresponding change is deployed in Jetpack, otherwise it would start breaking. So first would be to deploy the change that bypasses Gutenberg blocks, and then we can stop disabling it.

The second part would be to add the Jetpack Markdown block.

Why do we need to add a filter in Gutenberg to adjust the content?

[nuzzio]

Hi @mkaz thank you for your feedback.

Is there a target release for that or is it still in the works?

That change is working now and is part of a companion PR to this one: Automattic/jetpack#9357

Let me know if that does not answer your question.

Why do we need to add a filter in Gutenberg to adjust the content?

It is worth clarifying that the intention is to adjust the content before it is served up to a reader. The logic here is that since there is a filter with priority 8 which runs here:

gutenberg/lib/compat.php

Line 225 in 049645b

function gutenberg_wpautop( $content ) {

It does a simple does post contain blocks check (return false !== strpos( $content, '<!-- wp:' );) and if it does contain blocks then it bypasses wpautop.

I added a filter in WPCom_Markdown that runs at priority 6 which runs the wpautop only the contents inside the markdown blocks here Automattic/jetpack@ae7537c

This seemed to be reasonable solution since we only need to run that filter when Gutenberg is installed , active and the post contains the markdown block.

An additional benefit could be that this would allow other Gutenberg blocks to modify their content as needed before displaying it to the readers.

think this would need to be done in multiple stages

That is a good point. So perhaps:

1. Jetpack hooks and functions to bypass Gutenberg blocks on Posts insert/update when Jetpack Markdown is on.
2. Gutenberg changes in this PR
3. Jetpack Markdown block.

What do you think?

[mkaz]

An additional benefit could be that this would allow other Gutenberg blocks to modify their content as needed before displaying it to the readers.

This doesn't sound quite right for your scenario, since the filter is on the post going into the editor, not to be displayed to the user. So for the markdown block, I don't think you want to run autop on the content going into the editor but only convert from markdown to HTML when displayed to the user.

You can simply store the raw markdown in the block some what equivalent to a code block, then the conversion can all happen on display.

Let me know if I'm missing something.

[nuzzio]

Haha 🤕 I feel a bit foolish, you are correct. My apologies, I misunderstood your question. I thought of posts that are adjusted prior to going to the user for which I use the the_content filter: Automattic/jetpack@ae7537c

Your question is why do we need the after_gutenberg_gets_post_to_edit filter.

My reasoning here was that I needed something to filter the post_content in the post being retrieved for edit, and swap the post_content for the post_content_filtered which is where the Markdown lives. Gutenberg uses the WordPress REST API to retrieve posts to edit which bypasses this functionality.

I used a filter to keep Jetpack concerns in Jetpack and just create an event that could be consumed by plugins which need to change the content of the post when being served to the Gutenberg editor.

[nuzzio]

@mkaz Thanks for bringing this up, I looked at the code again following our conversation and realized that I needed to push up some fixes in Jetpack after I moved the code from Gutenberg and made it a filter:

• Automattic/jetpack@0e1cd99
• Automattic/jetpack@d8bf411

[nuzzio]

@ehg @mkaz I implemented a more robust way to check Markdown compatibility which I hope will enable a smoother merge. I would be grateful for any feedback you have on this. Thanks :)

The implementation consists of a static flag on the Jetpack Markdown module here: 22709a5
And a check in the Gutenberg plugin-compat.php here: 22709a5

[mkaz]

Adding a filter and only modify if not compatible seems like a good approach. Easier to follow what is going on.

[aduth]

At the risk of reigniting a bikeshed, I'd suggest avoiding the codename gutenberg in the names of hooks. See also #4681.

[nuzzio]

Replaced gutenberg with block_editor.

[aduth]

Should this if be collapsed into its parent since it's effectively redundant nesting?

// Before:
if ( $foo > 10 ) {
	if ( $foo > 100 ) {
		// ...
	}
}

// After:
if ( $foo > 10 && $foo > 100 ) {
	// ...
}

[nuzzio]

I agree that they should be collapsed. Fixed.

[aduth]

I suspect there may be conflicts between this and #6384, where the post is no longer prepared in client-assets.php for the editor (cc @youknowriad). In general, we probably don't want to rely on any behaviors where a post is expected to come into existence on the server (in future iterations, a post may be optional for initializing the editor).

[nuzzio]

I think the favored approach to the Markdown block is going to be client-side parsing. So it does not seem that this PR will be needed for that.

[ehg]

I think the favored approach to the Markdown block is going to be client-side parsing. So it does not seem that this PR will be needed for that.

Yep. Thanks for all your work on this René. Let's close this.