Opened 6 years ago
Last modified 4 weeks ago
#48054 reviewing enhancement
wp.sanitize.stripTags should iterate instead of using recursion
Reported by: |
|
Owned by: |
|
---|---|---|---|
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.
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)
Change History (6)
#1
@
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
.
#3
@
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
@
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
- Add the code provided in the artifacts to a plugin or functions.php or wherever it can be executed
- Remember to build JS files before and after testing
- Add the shortcode
[wp_sanitize_demo]
to a post - Check if the results are identical before and after the patch.
Actual Results
- ✅ 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; }
applying changes to /js/_enqueues/wp/sanitize.js