Automattic / syntaxhighlighter

WordPress plugin that makes it easy to post syntax-highlighted code snippets.
https://alex.blog/wordpress-plugins/syntaxhighlighter/
239 stars 78 forks source link

Escape nested shortcodes to block WP from running them #260

Closed fjorgemota closed 1 year ago

fjorgemota commented 1 year ago

Fixes #32

Changes proposed in this Pull Request

Testing instructions

First of all, setup your WP environment so:

// Register the shortcode
function current_datetime_shortcode() {
    return date( 'Y-m-d H:i:s' );
}
add_shortcode( 'current_datetime', 'current_datetime_shortcode' );

// Register the shortcode
function uppercase_content_shortcode( $atts, $content = null ) {
    if ( $content ) {
        return strtoupper( $content );
    }
}
add_shortcode( 'yell', 'uppercase_content_shortcode' );

Now, create a post (in both classic editor as well as on the block editor), a comment, a text widget (apparently this block supports them?), well, everywhere that this plugin supports highlighting code on, and with the following code:

#include <stdio.h>
int main() {
    char** data = {"hello world"};
    int c = 0;
    printf(data[c]);
    return 0;
    // A [current_datetime]
    // B [[current_datetime]]
    // C [[c]]
    // D [yell]is this yelling?[/yell]
    // E [[yell]]hey there??[[/yell]]
}

(If using shortcodes, remember to test both [c] ... [/c] as well as [code lang="c"] ... [/code] as an option)

Verify if the highlighted code generated by the plugin is effectively the same as the one above, including checking if the number of [ and ] is equal. Also, verify that the "shortcodes" inside the code aren't being run anymore.

jom commented 1 year ago

This seems to prevent shortcodes on the rest of the page as well. Example post:

<!-- wp:paragraph -->
<p>Real Shortcode:</p>
<!-- /wp:paragraph -->

<!-- wp:shortcode -->
[current_datetime]
<!-- /wp:shortcode -->

<!-- wp:paragraph -->
<p>SyntaxHighlighter Shortcode:</p>
<!-- /wp:paragraph -->

<!-- wp:shortcode -->
[php light="true"]datetime: [current_datetime][/php]
<!-- /wp:shortcode -->
fjorgemota commented 1 year ago

Hey there @jom, thanks so much for your review, good catch!

While working on this, I didn't notice that I was actually escaping all the shortcodes for all the content of the posts. My fauly, really.

I fixed this mainly in eefd331, but also sent d5dd900, b2c3462 and 46d5470 to improve the general behavior of that approach. Unfortunately, to fix this I need to use a private property temporarily to save all the known shortcodes, so...it is not beautiful. I'm eager to accept better suggestions here. ;)

Thanks!

aaronfc commented 1 year ago

Since I am not goind to look into this in the short-term I added Evergreen as a reviewer. I will try to look into it probably next week if no-one has looked into this yet :)

fjorgemota commented 1 year ago

So, after struggling in the last few days to get a working solution, I found out that this problem is pretty complicated to solve properly. There are a few reasons for this:

  1. The syntax highlighting logic (more specifically, its shortcodes) run very early in the pipeline. The goal is for them to run (partially, that is, with only the shortcodes implemented by this plugin) BEFORE wpautop, which apparently breaks all the code passed to this plugin, using a method internally called as shortcode_hack. However, this causes it to also run before do_blocks, and also do_shortcode, which means that we might end up running shortcodes more than once.
  2. Given that shortcodes in WP are based on regular expressions, we can't infer the context properly. I tried to implement a quite ugly hack to detect that by detecting whenever WP was trying to run a shortcode from this plugin, saving its content (aka the code being highlighted), and using strpos to detect references to shortcodes inside the code, and just returns them early.
  3. Even when the shortcode logic is running properly, it's hard to make the block work well: Given that the content and arguments passed to the block are encoded inside the post content, the shortcode logic parses and execute all the "shortccodes" (even when they aren't shortcodes) INSIDE the blocks, which make everything weirder.

I thought about maybe implementing a system to, instead of executing the highlighting shortcode in the shortcode_hack (hence running before wpautop and the blocks system), we just... save the content of each shortcode inside an array associated with an ID and passed that ID as a parameter for the shortcode itself, to use as a placeholder, so it can run properly on do_shortcode.

But the truth is: I tried way too many changes in the past few days, and all of them failed in one way or another, which makes this a very hard issue to work on, at least without making some huge possibly-breaking changes in the plugin, which I don't think is THAT desirable given how popular it is.

Because of this, I moved this PR back to draft. I might work on this in the future, but if someone wants to start working on it, feel free to do it. I'm open to discussing this further, too. :)

gogdzl commented 1 year ago

Because of this, I moved this PR back to draft. I might work on this in the future, but if someone wants to start working on it, feel free to do it. I'm open to discussing this further, too. :)

Hello, @fjorgemota o/

I attempted a different approach to tackle issue #32. During my testing, I also encountered problems with issues #26 and #71 (both seem to have the same root cause). Additionally, I stumbled upon some unreported bugs when mixing blocks and shortcodes. I'll be opening detailed issues about those later.

I've opened a PR (#261) to address both the problems, and I've made an effort to keep the solution simple and minimize any performance impact. I'm really happy to help! I hope this helps!

fjorgemota commented 1 year ago

Thanks for the PR, @gogdzl.

Closed in favor of #261, then.