Skip to content

Markdown: Reverse text hashes before restoration#7536

Merged
oskosk merged 1 commit intoAutomattic:masterfrom
rmccue:reverse-hashes-before-restore
Dec 20, 2017
Merged

Markdown: Reverse text hashes before restoration#7536
oskosk merged 1 commit intoAutomattic:masterfrom
rmccue:reverse-hashes-before-restore

Conversation

@rmccue
Copy link
Copy Markdown
Contributor

@rmccue rmccue commented Jul 26, 2017

This ensures that nested blocks are replaced first (e.g. code inside shortcodes).

Fixes #7535.

Changes proposed in this Pull Request:

  • Replace hashes in (roughly) the same order they were added.

Testing instructions:

Proposed changelog entry for your changes:

  • Markdown: Fixed bug where code inside shortcodes isn't correctly restored from the hash.

@rmccue
Reverse text hashes before restoration
c40109a 
This ensures that nested blocks are replaced first (e.g. code inside
shortcodes).
@rmccue rmccue requested a review from a team as a code owner July 26, 2017 08:09
@kraftbj kraftbj added [Feature] Markdown [Pri] Normal [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Jul 26, 2017
@kraftbj kraftbj requested a review from mattwiebe July 26, 2017 14:00
@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented Jul 26, 2017 •
edited
Loading

This makes sense and is reasonable to me. @mattwiebe can you take a look? Remembered that he's AFK.

@kraftbj kraftbj removed the request for review from mattwiebe July 26, 2017 14:02
@eliorivero
Copy link
Copy Markdown
Contributor

eliorivero commented Jul 28, 2017

This solves the issue that intends to solve. I'm concerned since honestly I know nothing about markdown and don't know if it might affect other areas.
Maybe @pento or @georgestephanis can verify this?

Interestingly, I also tested this with

[test]Text with `code` in it.[/test]

and the word in backticks wasn't wrapped in code tags. It's not introduced by this PR since it happens in master as well. I guess it's beyond the scope of this PR so I opened this new issue #7560.

@pento
Copy link
Copy Markdown
Contributor

pento commented Jul 31, 2017

Reversing the array direction should do the job.

@pento
Copy link
Copy Markdown
Contributor

pento commented Jul 31, 2017

needs-unit-tests 😁

oskosk
oskosk approved these changes Dec 20, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM. Tests well

@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Dec 20, 2017

Merging as it was reviewed long ago by others.

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Dec 20, 2017
@oskosk oskosk merged commit 9dd2cc1 into Automattic:master Dec 20, 2017
@oskosk oskosk added this to the 5.7 milestone Dec 20, 2017
shogo82148 added a commit to shogo82148/jetpack-markdown that referenced this pull request Jun 19, 2019
@shogo82148
Markdown: Reverse text hashes before restoration
585598b 
Automattic/jetpack#7536
@shogo82148 shogo82148 mentioned this pull request Jun 19, 2019
Merged
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@oskosk oskosk oskosk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Markdown [Pri] Normal Touches WP.com Files

Projects

None yet

Milestone

5.7

Development

Successfully merging this pull request may close these issues.

Markdown: MARKDOWN_HASH appearing in output

6 participants

@rmccue @kraftbj @eliorivero @pento @oskosk @matticbot