Make WordPress Core

Opened 6 years ago

Last modified 4 weeks ago

#48054 reviewing enhancement

wp.sanitize.stripTags should iterate instead of using recursion

Reported by: jrchamp's profile jrchamp Owned by: flixos90's profile flixos90
Milestone: 6.9 Priority: normal
Severity: minor Version: 5.2.3
Component: General Keywords: has-patch has-test-info
Focuses: javascript, performance Cc:

Description

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.

Attachments (1)

48054.patch (1.2 KB) - added by sabernhardt 5 years ago.
applying changes to /js/_enqueues/wp/sanitize.js

Download all attachments as: .zip

Change History (6)

@sabernhardt
5 years ago

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

#1 @sabernhardt
5 years ago

  • Focuses javascript performance added
  • Keywords needs-testing added

@jrchamp Thanks for the patch!

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

#2 @jrchamp
5 years ago

Thanks @sabernhardt!

#3 @flixos90
5 weeks ago

  • Milestone changed from Awaiting Review to 6.9
  • Owner set to flixos90
  • Status changed from new to reviewing

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 ticket was mentioned in PR #8980 on WordPress/wordpress-develop by @SirLouen.


4 weeks ago
#4

This is the refresh

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

#5 @SirLouen
4 weeks ago

  • Keywords has-test-info added; needs-testing removed

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;
} 

Note: See TracTickets for help on using tickets.