wp.sanitize.stripTags should iterate instead of using recursion

[jrchamp]

The changes to wp-sanitize.js seemed wasteful and like it produced dead code in the final return.

​https://github.com/WordPress/WordPress/commit/90d9bdc54c82229b3aa5a6e60d775f14b1f9a9dc#diff-9bedaa5d6abc6cb127b2f496bb2f7835

When I got into it to fix the dead code, I also felt a way about the recursion, because stack and memory exhaustion are two other potential issues. So this way should *at most* be one function call and only duplicate memory instead of being O(n*memory).

​https://github.com/WordPress/WordPress/commit/d9b128f50546a65c9cf0944ee61237e35ff269c0

This is probably a drive-by commit and for that I apologize. I hope it's more useful than not, but your mileage may vary. Thank you for making WordPress.

[sabernhardt]

applying changes to /js/_enqueues/wp/sanitize.js

[sabernhardt]

@jrchamp Thanks for the patch!

I simply moved those changes to the file that creates wp-includes/js/wp-sanitize.js.

[jrchamp]

Thanks @sabernhardt!

[flixos90]

Reviewing this as part of a bug scrub, it feels rather trivial to commit. Looks like nobody followed up here all the years, but it should be a small quick win to add.

This is the refresh

Trac ticket: https://core.trac.wordpress.org/ticket/48054

[SirLouen]

Test Report

Description

✅ This report validates that the indicated patch works as expected.

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/8980.diff

Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 8.2.28
• Server: nginx/1.27.5
• Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
• Browser: Chrome 137.0.0.0
• OS: Windows 10/11
• Theme: Twenty Twenty-Five 1.2
• MU Plugins: None activated
• Plugins:
  • Test Reports 1.2.0

Testing Instructions

1. Add the code provided in the artifacts to a plugin or functions.php or wherever it can be executed
2. Remember to build JS files before and after testing
3. Add the shortcode [wp_sanitize_demo] to a post
4. Check if the results are identical before and after the patch.

Actual Results

1. ✅ Patch works correctly, as expected

Additional Notes

I agree with @jrchamp, the recursion here seems completely unnecessary adds stack risks, also recursion is always more difficult to understand, and it's self-contained after all. I cannot see any advantages of using here the recursion version.

Anyway, I would have preferred here to see trouble in action, other than just a code refactor suggestion as code refactors are not generally well seen (and only up to be taken by certain committers that seem to be happy that day).

Supplemental Artifacts

Some code aimed for testers to test that works after and before:

add_shortcode( 'wp_sanitize_demo', 'wp_sanitize_demo_shortcode' );

function wp_sanitize_demo_shortcode() {
    wp_enqueue_script( 'wp-sanitize' );
    $html = '<div class="test"><script>alert("test");</script><p>This is a <b>test</b> with <i>HTML</i> tags</p> to be stripped</div>';
    
    $output = '<div id="wp-sanitize-demo">';
    $output .= '<h3>Original HTML:</h3>';
    $output .= '<pre>' . esc_html($html) . '</pre>';
    $output .= '<h3>Sanitized Result:</h3>';
    $output .= '<div id="sanitized-result"></div>';
    $output .= '<script>
        document.addEventListener( "DOMContentLoaded", function() {
            var original = ' . json_encode($html) . ';
            var sanitized = wp.sanitize.stripTags(original);
            document.getElementById("sanitized-result").textContent = sanitized;
        });
    </script>';
    $output .= '</div>';
    
    return $output;
} 

@SirLouen I tested the shortcode you shared in the testing instructions https://core.trac.wordpress.org/ticket/48054#comment:5, but I didn’t notice any change in the HTML output.

Could you please share a bit more detail about what should change, or provide a before/after screenshot if possible?

[SirLouen]

@mukesh27, as far as I can remember, this was just a refactor. The test is basically that: everything still in place after and before the patch with a code that actually executes the section of code that is being changed.

[rollybueno]

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/8980

Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 8.2.29
• Server: nginx/1.29.1
• Database: mysqli (Server: 8.4.6 / Client: mysqlnd 8.2.29)
• Browser: Chrome 140.0.0.0
• OS: Linux
• Theme: Twenty Twenty-Five 1.2
• MU Plugins: None activated
• Plugins:
  • Gutenberg 21.7.0
  • Test Reports 1.2.0

Actual Results

1. ✅ Issue resolved with patch, if we just expecting same result from trunk.

Additional Notes

• Patch was fetched and applied locally using:

  git fetch upstream pull/8980/head:pr-8980
  git checkout pr-8980
  

• Used test code from https://core.trac.wordpress.org/ticket/48054#comment:5
• No errors encountered during build or test execution.

Supplemental Artifacts

trunk as of this report:
PR 8980:

Could you please share a bit more detail about what should change, or provide a before/after screenshot if possible?

@mukeshpanchal27 There should not be any difference. It's purely a memory optimization and it prevents a possible stack overflow.

[westonruter]

In 60907:

General: Optimize wp.sanitize.stripTags() to improve memory usage.

This updates the logic to use iteration instead of recursive calls, resulting in reduced memory usage and avoiding a possible stack overflow for large HTML documents.

The JS code is also tidied up with improved JSDoc and utilizing let instead of var.

Props jrchamp, sabernhardt, SirLouen, rollybueno, mukesh27, flixos90, westonruter.
Fixes #48054.

[westonruter]

In 61347:

General: Leverage DOMParser to implement wp.sanitize.stripTags().

Developed in ​https://github.com/WordPress/wordpress-develop/pull/10536

Follow-up to [60907].

Props hbhalodia, dmsnell, westonruter.
See #48054.
Fixes #64274.