Skip to content

Icons: Use snake_case file_path key in icon registry#79100

Open
t-hamano wants to merge 3 commits into
trunkfrom
icons/snake-case
Open

Icons: Use snake_case file_path key in icon registry#79100
t-hamano wants to merge 3 commits into
trunkfrom
icons/snake-case

Conversation

@t-hamano

@t-hamano t-hamano commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Part of #75715

What?

Rename the camelCase filePath key to snake_case file_path in the PHP icon registry.

Why?

The PHP-side icon path was exposed under a camelCase filePath key, which is inconsistent with PHP/WordPress array-key conventions.

Furthermore, with #77260, we are attempting to build the foundation for custom icon registration. By shipping this PR, I expect to change the icon registration method as follows:

// Before
  wp_register_icon( 'star', 'my-icons', array(
      'label'    => 'Star',
      'filePath' => './path/to/icon.svg',
  ) );

// After
  wp_register_icon( 'star', 'my-icons', array(
      'label'     => 'Star',
      'file_path' => './path/to/icon.svg',
  ) );

How?

This PR maintains full backward compatibility regardless of whether the base class (WP_Icons_Registry) exists in core:

  • If the base class is absent (WP < 7.0): lib/compat/wordpress-7.0/class-wp-icons-registry.php defines the class with the file_path key.
  • If the base class is present (shipped by core): the compat class is skipped by the class_exists guard, but WP_Icons_Registry_Gutenberg::get_content() overrides content retrieval to read from file_path, so it does not break even if core's base class still keys on filePath.

Testing Instructions

  • Confirm that the Icon block works correctly in both the editor and the frontend.
  • Confirm that the Icon block works even if the WordPress version is downgraded to 6.9.

Use of AI Tools

This PR was authored with assistance from Claude Code. All changes were reviewed by the author.

The generated `manifest.php` and the PHP icon registry exposed the icon
path under a camelCase `filePath` key, which is inconsistent with PHP/WP
array-key conventions. Rename it to `file_path` across the manifest PHP
generator, both registry classes, and the test docblock. The
`manifest.json` source intentionally keeps `filePath` (JS convention).

Because the Gutenberg override inherits `get_content()` from the base
class, also override `get_content()` in `WP_Icons_Registry_Gutenberg` so
content is read from the `file_path` property even when the base class
comes from WordPress core (which may still use `filePath`), preventing a
key mismatch that would silently break icon content retrieval.

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added the [Package] Icons /packages/icons label Jun 11, 2026
@github-actions

Copy link
Copy Markdown

Size Change: 0 B

Total Size: 8.46 MB

compressed-size-action

@t-hamano t-hamano added [Type] Code Quality Issues or PRs that relate to code quality [Feature] Icons Related to Icon registration API and Icon REST API labels Jun 11, 2026
t-hamano added a commit to t-hamano/wordpress-develop that referenced this pull request Jun 11, 2026
Align the icon registry and generated manifest with WordPress core's
snake_case array-key convention, which is the dominant style across core
PHP. This updates the manifest entries, the registry validation and
`get_content()` lookup, the allowed property keys, and the related
docblocks and error messages.

Mirrors the same rename made on the Gutenberg side in
WordPress/gutenberg#79100.

Co-Authored-By: Claude <noreply@anthropic.com>
@t-hamano t-hamano marked this pull request as ready for review June 11, 2026 02:52
@t-hamano t-hamano requested a review from spacedmonkey as a code owner June 11, 2026 02:52
@t-hamano t-hamano self-assigned this Jun 11, 2026
@github-actions

github-actions Bot commented Jun 11, 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.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

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

t-hamano added a commit to t-hamano/wordpress-develop that referenced this pull request Jun 11, 2026
Following the upstream Gutenberg rename of the icon manifest key from
filePath to file_path (WordPress/gutenberg#79100), adjust the
copy:icon-library-manifest Grunt task to match the new file_path key
when stripping the 'library/' path prefix, so regeneration keeps
producing valid icon paths.

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

Copy link
Copy Markdown

Flaky tests detected in c089786.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/27320567789
📝 Reported issues:

@t-hamano t-hamano changed the title Icons: Use snake_case file_path key in PHP icon manifest and registry Icons: Use snake_case file_path key in icon registry Jun 11, 2026
… in registry

The previous commit renamed the icon path key to snake_case file_path
across both the generated manifest.php and its generator. The manifest
mirrors the JS manifest.json, which uses camelCase filePath, so the
generated PHP manifest should keep filePath for consistency with its
source. Revert the generator and manifest.php back to filePath.

The PHP registry still exposes the path as file_path (PHP/WP array-key
convention), so register_collection now reads the camelCase filePath
from the manifest and converts it to file_path when registering. This
confines the filePath-to-file_path conversion to the registry boundary.

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions github-actions Bot removed the [Package] Icons /packages/icons label Jun 11, 2026
t-hamano added a commit to t-hamano/wordpress-develop that referenced this pull request Jun 11, 2026
Align the icon registry and generated manifest with WordPress core's
snake_case array-key convention, which is the dominant style across core
PHP. This updates the manifest entries, the registry validation and
`get_content()` lookup, the allowed property keys, and the related
docblocks and error messages.

Mirrors the same rename made on the Gutenberg side in
WordPress/gutenberg#79100.

Co-Authored-By: Claude <noreply@anthropic.com>
t-hamano added a commit to t-hamano/wordpress-develop that referenced this pull request Jun 11, 2026
Following the upstream Gutenberg rename of the icon manifest key from
filePath to file_path (WordPress/gutenberg#79100), adjust the
copy:icon-library-manifest Grunt task to match the new file_path key
when stripping the 'library/' path prefix, so regeneration keeps
producing valid icon paths.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment on lines +174 to +199
/**
* Redefined to read the icon content from the `file_path` property.
*
* @param string $icon_name Icon name including namespace.
* @return string|null The content of the icon, if found.
*/
protected function get_content( $icon_name ) {
if ( ! isset( $this->registered_icons[ $icon_name ]['content'] ) ) {
$content = file_get_contents(
$this->registered_icons[ $icon_name ]['file_path']
);
$content = $this->sanitize_icon_content( $content );

if ( empty( $content ) ) {
wp_trigger_error(
__METHOD__,
__( 'Icon content does not contain valid SVG markup.', 'gutenberg' )
);
return null;
}

$this->registered_icons[ $icon_name ]['content'] = $content;
}
return $this->registered_icons[ $icon_name ]['content'];
}

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.

Why was this function added? It seems to expand the scope of this PR beyond its original purpose.

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 is a bit tricky, but if the Gutenberg plugin is active on WordPress 7.0, the WP_Icons_Registry base class already exists in core, so the if statement here evaluates to false. This means the old get_content method, which relies on an outdated filePath, will be used, causing unintended behavior. To prevent this, we need to completely override this method on the Gutenberg side.

Comment on lines 85 to +89
* @type string $label Required. A human-readable label for the icon.
* @type string $content Optional. SVG markup for the icon.
* If not provided, the content will be retrieved from the `filePath` if set.
* If both `content` and `filePath` are not set, the icon will not be registered.
* @type string $filePath Optional. The full path to the file containing the icon content.
* If not provided, the content will be retrieved from the `file_path` if set.
* If both `content` and `file_path` are not set, the icon will not be registered.
* @type string $file_path Optional. The full path to the file containing the icon content.

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.

need some adjustments

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.

What adjustments are needed?

}

$allowed_keys = array_fill_keys( array( 'label', 'content', 'filePath' ), 1 );
$allowed_keys = array_fill_keys( array( 'label', 'content', 'file_path' ), 1 );

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.

What happens if I have been using filePath in my plugin? Should we add a compat layer for that (a _doing_it_wrong() + a graceful transformation to file_path?

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.

+1

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.

What happens if I have been using filePath in my plugin?

WP_Icons_Registry does not currently allow the registration of third-party icons, and its register method is protected. Therefore, it is not fundamentally intended to be executed externally unless reflection methods are used. This means that, in practice, filePath should not be used externally. Consequently, we believe a compat layer is unnecessary.

In the 7.1 release, I intend to make the register method public to allow third-party icons to be registered. Since changing parameter names will be difficult once the method is public, I wanted to address this issue now.

*
* @param string $icon_name Icon name including namespace.
* @param array $icon_properties Icon properties (label, content, filePath).
* @param array $icon_properties Icon properties (label, content, file_path).

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.

Should we add some tests that test this prop? I found it unexpected that in tests we only updated the docs.

protected function get_content( $icon_name ) {
if ( ! isset( $this->registered_icons[ $icon_name ]['content'] ) ) {
$content = file_get_contents(
$this->registered_icons[ $icon_name ]['file_path']

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.

We're directly reading the file, meaning we're missing to check if it exists, if it's readable, etc. We're also not really triggering an error early enough if there's an error with readability/access of the file itself. We might want to introduce a is_readable(), otherwise this may also trigger warnings. The empty( $content ) check down below can remain, it's a separate concern - the file may be readable but be empty or fail to sanitize as a valid icon.

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.

Thank you for noticing. This issue is likely to occur more frequently when custom icon registration is allowed, so I would like to address it in #77260.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Icons Related to Icon registration API and Icon REST API [Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants