Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions backport-changelog/7.2/12197.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
https://github.com/WordPress/wordpress-develop/pull/12197

* https://github.com/WordPress/gutenberg/pull/78808
* https://github.com/WordPress/gutenberg/pull/82846
31 changes: 29 additions & 2 deletions lib/class-wp-icons-registry-gutenberg.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,33 @@ function gutenberg_get_allowed_icon_svg_tags(): array {
),
$stroke_attributes
),
'rect' => array_merge(
array(
'fill' => true,
'fill-rule' => true,
'clip-rule' => true,
'x' => true,
'y' => true,
'width' => true,
'height' => true,
'rx' => true,
'ry' => true,
'transform' => true,
),
$stroke_attributes
),
'circle' => array_merge(
array(
'fill' => true,
'fill-rule' => true,
'clip-rule' => true,
'cx' => true,
'cy' => true,
'r' => true,
'transform' => true,
),
$stroke_attributes
),
);
}

Expand Down Expand Up @@ -260,8 +287,8 @@ public function unregister( $icon_name ) {
/**
* Sanitizes the icon SVG content.
*
* Overrides the base class to allow stroke-related attributes and inline
* styles required by stroke-based icons.
* Overrides the base class to allow the `rect` and `circle` shapes, plus the
* stroke-related attributes and inline styles required by stroke-based icons.
*
* The signature is intentionally left without type declarations to stay
* compatible with the parent WP_Icons_Registry::sanitize_icon_content()
Expand Down
94 changes: 94 additions & 0 deletions phpunit/class-wp-icons-registry-gutenberg-test.php
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,100 @@ public function test_clip_rule_survives_sanitization( bool $use_file_path ) {
$this->assertStringContainsString( 'clip-rule="evenodd"', $icon['content'] );
}

/**
* Should preserve `rect` and `circle` shapes when sanitizing registered icons.
*
* @dataProvider data_icon_content_sources
*
* @param bool $use_file_path Whether to register the icon from a file path.
*/
public function test_rect_and_circle_survive_sanitization( bool $use_file_path ) {
$rect = '<rect x="4" y="5" width="16" height="14" rx="2" ry="2" fill="currentColor" stroke="currentColor" transform="rotate(45)" vector-effect="non-scaling-stroke" />';
$circle = '<circle cx="12" cy="12" r="3" fill="currentColor" fill-rule="evenodd" clip-rule="evenodd" transform="rotate(45)" vector-effect="non-scaling-stroke" />';
$content = '<svg viewBox="0 0 24 24" style="fill: none" stroke="currentColor" stroke-width="1.5">' . $rect . $circle . '</svg>';
$name = 'test-collection/shapes-icon';
$settings = array(
'label' => 'Shapes Icon',
);

if ( $use_file_path ) {
$settings['file_path'] = $this->create_temp_icon_file( $content );
} else {
$settings['content'] = $content;
}

$this->assertTrue( $this->register( $name, $settings ) );

$icon = $this->registry->get_registered_icon( $name );
$this->assertStringContainsString( $rect, $icon['content'] );
$this->assertStringContainsString( $circle, $icon['content'] );
}

/**
* Provides every SVG file shipped in the `@wordpress/icons` library.
*
* @return array<string, array{0: string}> Data sets of [ $file_path ], keyed by icon slug.
*/
public function data_library_icons(): array {
$data = array();
foreach ( glob( gutenberg_dir_path() . 'packages/icons/src/library/*.svg' ) as $file_path ) {
$data[ basename( $file_path, '.svg' ) ] = array( $file_path );
}
return $data;
}

/**
* Should keep every element and attribute of a library icon through sanitization.
*
* @dataProvider data_library_icons
*
* @param string $file_path Absolute path to the library SVG file.
*/
public function test_library_icon_survives_sanitization( string $file_path ) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While I agree that this test is useful, it seems to go well beyond the scope of this PR. Furthermore, if this test is truly necessary, we should directly test the sanitize_icon_content() method using a reflection class, rather than relying on the register() and get_registered_icon() methods (see here for reference). Could we remove this test from this PR for now, and handle testing for the icon package in a separate PR?

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.

SGTM, Aki.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@retrofox, what do you think about completely removing this test in this PR? I have already prepared a separate dedicated PR for similar tests. #83008

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.

It makes sense to me, @t-hamano. Let me do it.

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.

Only test_library_icon_survives_sanitization or all of them?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we just need to delete data_library_icons(), get_svg_structure(), and test_library_icon_survives_sanitization(). I think we can keep test_rect_and_circle_survive_sanitization().

$name = 'test-collection/' . basename( $file_path, '.svg' );

$this->assertTrue(
$this->register(
$name,
array(
'label' => 'Library Icon',
'file_path' => $file_path,
)
)
);

$icon = $this->registry->get_registered_icon( $name );
$this->assertSame(
$this->get_svg_structure( file_get_contents( $file_path ) ),
$this->get_svg_structure( $icon['content'] )
);
}

/**
* Lists the tags of an SVG document with their attributes, in document order.
*
* Tag names and attribute names are compared case-insensitively because
* `wp_kses()` lowercases attribute names such as `viewBox`.
*
* @param string $svg SVG markup.
* @return array<int, array{0: string, 1: array<string, string|true|null>}> Tag name and attributes per element.
*/
private function get_svg_structure( string $svg ): array {
$structure = array();
$processor = new WP_HTML_Tag_Processor( $svg );

while ( $processor->next_tag() ) {
$attributes = array();
foreach ( $processor->get_attribute_names_with_prefix( '' ) as $attribute_name ) {
$attributes[ $attribute_name ] = $processor->get_attribute( $attribute_name );
}
ksort( $attributes );
$structure[] = array( $processor->get_tag(), $attributes );
}

return $structure;
}

/**
* Should fail to register an icon that provides both `content` and `file_path`.
*
Expand Down
Loading