Skip to content

Icons: Allow additional SVG elements and attributes in the icon sanitizer - #13559

Closed
t-hamano wants to merge 4 commits into
WordPress:trunkfrom
t-hamano:65795-icons/allow-stroke-svg-elements
Closed

t-hamano wants to merge 4 commits into
WordPress:trunkfrom
t-hamano:65795-icons/allow-stroke-svg-elements

Conversation

@t-hamano

@t-hamano t-hamano commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

This PR is a subset of #12197, focusing only on allowing the SVG elements and attributes needed for core icons to render correctly.

Related Gutenberg PR:

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

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Porting the Gutenberg changes, writing tests, and drafting this description; reviewed by me.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@t-hamano t-hamano changed the title Icons: Allow stroke attributes and rect/circle elements in the icon SVG sanitizer Icons: Allow additional SVG elements and attributes in the icon sanitizer Sep 16, 2026
$this->assertStringNotContainsString( 'width=', $output );
$this->assertStringNotContainsString( 'height=', $output );
$output = wp_get_icon( 'core/plus', array( 'size' => null ) );
$processor = new WP_HTML_Tag_Processor( $output );

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test is updated because stroke-width is now preserved, so checking for the width= string would match it. The attributes are inspected directly instead.

…VG sanitizer.

Co-Authored-By: Claude <noreply@anthropic.com>
@t-hamano
t-hamano force-pushed the 65795-icons/allow-stroke-svg-elements branch from 0ca3a33 to 27b2d08 Compare September 16, 2026 11:59
@t-hamano
t-hamano marked this pull request as ready for review September 16, 2026 11:59
@github-actions

github-actions Bot commented Sep 16, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props wildworks, westonruter.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Comment on lines +251 to +259
$stroke_attributes = array(
'style' => true,
'stroke' => true,
'stroke-width' => true,
'stroke-linecap' => true,
'stroke-linejoin' => true,
'stroke-miterlimit' => true,
'vector-effect' => true,
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor: All of these associative arrays can be simplified by using array_fill_keys(), which eliminates the pains of aligning the =>.

Suggested change
$stroke_attributes = array(
'style' => true,
'stroke' => true,
'stroke-width' => true,
'stroke-linecap' => true,
'stroke-linejoin' => true,
'stroke-miterlimit' => true,
'vector-effect' => true,
);
$stroke_attributes = array_fill_keys(
array(
'style',
'stroke',
'stroke-width',
'stroke-linecap',
'stroke-linejoin',
'stroke-miterlimit',
'vector-effect',
),
true
);

I recall we did this for some other Kses tests already, or we talked about it.

Same goes for the following associative arrays as well.

A helper method could make this more concise, or even a closure in this method:

$generate_array = static function ( string ...$tag_names ): array {
	return array_fill_keys( $tag_names, true );
};

Then the above can be just:

		$stroke_attributes = $generate_array(
			'style',
			'stroke',
			'stroke-width',
			'stroke-linecap',
			'stroke-linejoin',
			'stroke-miterlimit',
			'vector-effect',
		);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice idea. That reminds me of our past discussion. Based on WordPress/gutenberg#75550 (comment), I have decided to use the get_allowed_attribute_list() private method.

Fixed in 706b8a8

Comment thread tests/phpunit/tests/icons/wpIconsRegistry.php Outdated
Comment thread tests/phpunit/tests/icons/wpIconsRegistry.php Outdated
Comment thread tests/phpunit/tests/icons/wpIconsRegistry.php Outdated
Comment thread tests/phpunit/tests/icons/wpIconsRegistry.php Outdated
Comment thread tests/phpunit/tests/icons/wpIconsRegistry.php Outdated
Comment thread tests/phpunit/tests/icons/wpIconsRegistry.php Outdated
t-hamano and others added 2 commits September 17, 2026 10:55
… with a helper method.

Co-Authored-By: Claude <noreply@anthropic.com>
…SVG sanitizer tests.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread src/wp-includes/class-wp-icons-registry.php Outdated

@westonruter westonruter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pre-approving.

…PDoc.

Co-Authored-By: Claude <noreply@anthropic.com>
t-hamano added a commit to WordPress/gutenberg that referenced this pull request Sep 17, 2026
* Icons: Test the registry sanitizer directly with a data provider

Mirror the tests from WordPress/wordpress-develop#13559: call `sanitize_icon_content` via reflection with a data provider instead of registering icons from both inline content and a file path. Replace the backport changelog entry for #12197 with one for #13559.

Co-Authored-By: Claude <noreply@anthropic.com>

* Icons: Align the SVG sanitizer and its tests with the WordPress core backport

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: mcsf <mcsf@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
@t-hamano

Copy link
Copy Markdown
Contributor Author

@t-hamano t-hamano closed this Sep 17, 2026
peterwilsoncc pushed a commit to peterwilsoncc/gutenberg-build that referenced this pull request Sep 17, 2026
* Icons: Test the registry sanitizer directly with a data provider

Mirror the tests from WordPress/wordpress-develop#13559: call `sanitize_icon_content` via reflection with a data provider instead of registering icons from both inline content and a file path. Replace the backport changelog entry for #12197 with one for #13559.

Co-Authored-By: Claude <noreply@anthropic.com>

* Icons: Align the SVG sanitizer and its tests with the WordPress core backport

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: mcsf <mcsf@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>

Source: WordPress/gutenberg@6db2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants