Fix error with Jetpack Markdown being enabled by mkaz · Pull Request #2817 · WordPress/gutenberg

mkaz · 2017-09-27T21:07:29Z

Description

When saving text blocks while Jetpack Markdown is enabled, the <p> tags are stripped on save. When loaded again in Gutenberg the blocks are not recognized since the tags are needed to properly parse paragraph blocks.

How to Test

Install Jetpack plugin in Settings, under Compose enable Markdown syntax
Create a new post with at least one text block and save
Click over to view the post and then click "Edit in Gutenberg"
The text block will have paragraph tags deleted and be asked to convert to classic block.

Change

This change disables markdown on content that is detected as Gutenberg by adding a filter using the rest_pre_insert_post hook.

aduth · 2017-09-27T22:06:33Z

gutenberg.php

The version bumps here shouldn't be part of the changes.

Rebased and removed.

aduth · 2017-09-27T22:07:10Z

lib/compat.php

I don't think that $settings would ever not be an array. What we might want to do here is test first whether the post contains blocks and, if it does, assign $settings['wpautop'] as explicitly false but otherwise do nothing with the key.

I like that, a bit cleaner.

aduth · 2017-09-27T22:07:56Z

lib/compat.php

Would it hurt if we removed wpcom-markdown support without explicitly testing whether we're in the context of Jetpack, since I assume the post support (by its prefix) is relevant for Jetpack only anyways?

I changed the method of determining and removed it regardless of JETPACK being defined.

aduth · 2017-09-27T22:10:08Z

lib/compat.php

post is not the only post type we'd expect to be using Gutenberg. Or is it the case that Jetpack is only adding support for this type? Otherwise we might have to hook into where Jetpack is testing the post type and check whether it's handled by Gutenberg. Currently we have a filter for this, but it's not very well generalized:

gutenberg/lib/register.php

Line 154 in 5a949b7

! apply_filters( 'gutenberg_add_edit_link_for_post_type', true, $post->post_type, $post ) ) {

I could imagine a function to call which runs the filter.

We also want to make sure that we're not removing this support if the post is being edited from the classic editor, since this action will be triggered regardless of whether the user is using the Gutenberg editor.

…d not flip settings if already set to true

This ends up being a bit heavy handed solution, since it will also disable markdown in the classic editor, however each block doesn't fail to classic blocks on each save. Is there a check we can add to init to see which editor is being used? The markdown processing is called during save a post, so not sure it is possible to determine or not

Use the pre insert filter on the rest endpoint to determine if the content being saved is from Gutenberg and disable the markdown support if a block is included. Temporary includes the content_has_block() function from PR#2806

Issues addressed in changes

aduth · 2017-10-09T19:59:50Z

lib/register.php

+ *
+ * @param string $content Content to test.
+ * @return bool Whether the content contains blocks.
+ */


If we're introducing this function, I think we should refactor gutenberg_post_has_blocks to use it as well, rather than repeat the strpos implementation.

Good catch, updated gutenberg_post_has_blocks to use the new function

aaronjorbin · 2017-10-09T20:27:47Z

lib/compat.php

+ * @param  array $post      Post object which contains content to check for block.
+ * @return array $post      Post object.
+ */
+function gutenberg_remove_wpcom_markdown_support( $post ) {


Shouldn't this live in the plugin that adds wpcom-markdown to posts?

@aaronjorbin it does makes sense to include in Jetpack. However, there are also other plugins, for example JP Markdown which include it.

@mkaz What about the other plugins that add markdown in a different way? Should the code for turning off markdown in each of them live in Gutenberg or should Gutenberg provide the tools (such as the content_has_blocks function and ideally some documentation) to assist them?

@aaronjorbin Gutenberg definitely should provide tools to assist, which this change does include one.

It's a bit of a catch-22, Gutenberg is still a plugin under development, trying to grow and get more people interested and gather feedback. If user wants to try it out and then their first experience is a broken, it's not a great way to gather feedback. So this fix disables a feature in one of the most popular plugins that makes for a poor Gutenberg experience.

As Gutenberg grows and gets closer to becoming the core editor, plugins will adopt the changes and include fixes themselves. However, as stated it is harder to grow if initial experience is rough.

Ideally, a plugin could provide a markdown block which would be awesome and any parsing conflict could also be avoided.

I completely agree on the catch-22, but I think it also goes other ways. If Gutenberg is going to be in core, it can't be responsible for changes to plugins. Either things need to be done in a fully backward compatible way, or education is needed for plugin maintainers to help them adjust. Playing favorites for popular plugins is going going to lead to unnecessary drama (what qualifies as a popular plugin? Why was that plugin given preference over mine?), an increased and unnecessary maintenance burdon for core, and slower fixes for users.

Perhaps what's needed is some documentation and a documentation standard for things that can't be included in a core merge if that was to happen?

codecov · 2017-10-09T20:28:45Z

Codecov Report

Merging #2817 into master will decrease coverage by 0.7%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2817      +/-   ##
==========================================
- Coverage   33.94%   33.23%   -0.71%     
==========================================
  Files         193      196       +3     
  Lines        5700     6059     +359     
  Branches     1000     1075      +75     
==========================================
+ Hits         1935     2014      +79     
- Misses       3185     3406     +221     
- Partials      580      639      +59

Impacted Files	Coverage Δ
blocks/library/gallery/block.js	`9.52% <0%> (-1.59%)`	⬇️
editor/block-settings-menu/index.js	`0% <0%> (ø)`	⬆️
editor/header/index.js	`0% <0%> (ø)`	⬆️
editor/table-of-contents/index.js	`0% <0%> (ø)`	⬆️
editor/sidebar/header.js	`0% <0%> (ø)`	⬆️
editor/writing-flow/index.js	`0% <0%> (ø)`	⬆️
editor/modes/visual-editor/block.js	`0% <0%> (ø)`	⬆️
editor/inserter/index.js	`0% <0%> (ø)`	⬆️
editor/header/mode-switcher/index.js	`0% <0%> (ø)`	⬆️
blocks/color-palette/index.js	`0% <0%> (ø)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3eec60...c202c68. Read the comment docs.

mkaz · 2017-10-09T22:29:49Z

@aaronjorbin - I added a file called plugin-compat.php which includes the plugin specific part of this change. I added documentation at the top which I hope captures our discussion.

Let me know what you think, see commit c202c68

Creating a new file as a means to better document compatibility support added for specific plugins. This change also isolates code that likely will not be included for a core merge.

aaronjorbin · 2017-10-10T00:30:25Z

@mkaz Looks good to me. Seems to achieve the goal of improving compatibility to help with testing while also making a core merge easier. Thanks for working on this and for making that change.

Fixed in 50461fe

mkaz added the [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible label Sep 27, 2017

aduth previously requested changes Sep 27, 2017

View reviewed changes

mkaz force-pushed the fix/jetpack-markdown-autop branch 3 times, most recently from 791667c to 30e6fd5 Compare October 2, 2017 21:07

mkaz removed the [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible label Oct 2, 2017

mkaz force-pushed the fix/jetpack-markdown-autop branch from a53e000 to 2836321 Compare October 2, 2017 23:32

mkaz added 5 commits October 3, 2017 09:42

Switch check for ! isset() to just check if array, current logic woul…

3995fdd

…d not flip settings if already set to true

Limit scope of disabling markdown support to just content

a6efdb1

Use the pre insert filter on the rest endpoint to determine if the content being saved is from Gutenberg and disable the markdown support if a block is included. Temporary includes the content_has_block() function from PR#2806

Limit scope of disabling markdown support to just content

f771140

Use the pre insert filter on the rest endpoint to determine if the content being saved is from Gutenberg and disable the markdown support if a block is included. Temporary includes the content_has_block() function from PR#2806

Move content_has_blocks to register.php to be compat with PR #2806

9377638

mkaz force-pushed the fix/jetpack-markdown-autop branch from 2836321 to 9377638 Compare October 3, 2017 16:43

aduth previously requested changes Oct 9, 2017

View reviewed changes

Refactor gutenberg_post_has_blocks to use content_has_blocks check

50461fe

aaronjorbin reviewed Oct 9, 2017

View reviewed changes

Move markdown fix to own plugin-compatibility file

c202c68

Creating a new file as a means to better document compatibility support added for specific plugins. This change also isolates code that likely will not be included for a core merge.

mkaz force-pushed the fix/jetpack-markdown-autop branch from 520671b to c202c68 Compare October 9, 2017 22:36

mkaz merged commit a59e2e4 into master Oct 10, 2017

mkaz deleted the fix/jetpack-markdown-autop branch October 10, 2017 18:25

aduth mentioned this pull request Oct 16, 2017

Set Gutenberg as the default editor #2951

Merged

4 tasks

pento added a commit that referenced this pull request Oct 18, 2017

Revert the revert of #2817.

2bfed02

This was referenced Oct 27, 2017

Compat: Disable wpautop for block-based posts #2806

Merged

Jetpack / Markdown text removed by Gutenberg #3207

Closed

mkaz added a commit that referenced this pull request Oct 31, 2017

Add back #2817 disabling of wpcom-markdown if blocks detected

e0a2238

mkaz mentioned this pull request Oct 31, 2017

Add back disabling of wpcom-markdown if blocks detected #3261

Merged

Conversation

mkaz commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How to Test

Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mkaz commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronjorbin commented Oct 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mkaz commented Sep 27, 2017 •

edited

Loading

codecov bot commented Oct 9, 2017 •

edited

Loading

mkaz commented Oct 9, 2017 •

edited

Loading