63714 - Ensure `wp_get_attachment_image()` doesn't overwrite an already valid user-provided width and height by heybran · Pull Request #9315 · WordPress/wordpress-develop

heybran · 2025-07-24T03:25:24Z

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

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.

Ensure `wp_get_attachment_image()` doesn't overwrite an already valid user-provided width and height.

github-actions · 2025-07-24T03:25:33Z

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 heybran, mukesh27, adamsilverstein.

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

github-actions · 2025-07-24T03:25:35Z

Hi @heybran! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook.

If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook.

The Developer Hub also documents the various coding standards that are followed:

Thank you,
The WordPress Project

github-actions · 2025-07-24T03:38:41Z

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

The Plugin and Theme Directories cannot be accessed within Playground.
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.

mukeshpanchal27 · 2025-07-24T04:48:40Z

src/wp-includes/media.php

+		$attr = wp_parse_args( $attr, $default_attr );
+
+		// Ensure that the `$width` doesn't overwrite an already valid user-provided width.
+		if ( is_numeric( $width ) && ( ! isset( $attr['width'] ) || ! is_numeric( $attr['width'] ) ) ) {


Let's check isset and numeric for $attr['width'] then numeric for $width as it always have numeric value.

Suggested change

if ( is_numeric( $width ) && ( ! isset( $attr['width'] ) || ! is_numeric( $attr['width'] ) ) ) {

if ( ( ! isset( $attr['width'] ) || ! is_numeric( $attr['width'] ) && is_numeric( $width ) ) ) {

Do same for height also.

Thanks for the review. I found $width and $height can be null in two of our projects when we're debugging the missing width/height attributes the other day, only for SVG images.

But I think in this case, if I'm not wrong, there is no need to check is_numeric( $width ) at all, checking ! isset( $attr['width'] ) || ! is_numeric( $attr['width'] ) alone should be enough here. I will take another closer look tomorrow. And I also wait and see if Adam has any further feedback.

You're right about the $width and $height always have numeric value, I tracked down the codes to set the width and height.
https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/media.php#L480-L482

The reason that I got null for the $width and $height is due to a filter we're using internally in the projects at the company that I work at. So all good there, thanks again.

mukeshpanchal27 · 2025-07-24T04:52:25Z

tests/phpunit/tests/media.php

+	public function test_wp_get_attachment_image_not_overwrite_user_provided_width_height() {
+		[$src, $width, $height] = wp_get_attachment_image_src( self::$large_id, 'large' );
+
+		$user_provided_width  = is_numeric( $width ) ? (int) $width + 1 : 999;
+		$user_provided_height = is_numeric( $height ) ? (int) $height + 1 : 199;
+
+		$img = wp_get_attachment_image(
+			self::$large_id,
+			'large',
+			false,
+			array(
+				'width'  => $user_provided_width,
+				'height' => $user_provided_height,
+			)
+		);
+
+		$expected_width_attribute  = 'width="' . $user_provided_width . '"';
+		$expected_height_attribute = 'height="' . $user_provided_height . '"';
+		$this->assertStringContainsString( $expected_width_attribute, $img, 'User-provided width should not be changed.' );
+		$this->assertStringContainsString( $expected_height_attribute, $img, 'User-provided height should not be changed.' );
+	}


Suggested change

public function test_wp_get_attachment_image_not_overwrite_user_provided_width_height() {

[$src, $width, $height] = wp_get_attachment_image_src( self::$large_id, 'large' );

$user_provided_width = is_numeric( $width ) ? (int) $width + 1 : 999;

$user_provided_height = is_numeric( $height ) ? (int) $height + 1 : 199;

$img = wp_get_attachment_image(

self::$large_id,

'large',

false,

array(

'width' => $user_provided_width,

'height' => $user_provided_height,

)

);

$expected_width_attribute = 'width="' . $user_provided_width . '"';

$expected_height_attribute = 'height="' . $user_provided_height . '"';

$this->assertStringContainsString( $expected_width_attribute, $img, 'User-provided width should not be changed.' );

$this->assertStringContainsString( $expected_height_attribute, $img, 'User-provided height should not be changed.' );

}

public function test_wp_get_attachment_image_not_overwrite_user_provided_width_height() {

$img = wp_get_attachment_image(

self::$large_id,

'large',

false,

array(

'width' => 999,

'height' => 999,

)

);

$this->assertStringContainsString( 'width="999"', $img, 'User-provided width should not be changed.' );

$this->assertStringContainsString( 'height="999"', $img, 'User-provided height should not be changed.' );

}

Simplified

Amazing, thank you @mukeshpanchal27

mukeshpanchal27 · 2025-07-24T04:53:38Z

Thanks @heybran for the PR.

I’ve left some feedback and suggestions. Please take a look and let me know if you need any clarification or further details.

mukeshpanchal27 · 2025-07-24T04:54:27Z

@adamsilverstein Could you please take a look when you have time?

adamsilverstein · 2025-07-25T02:56:35Z

Looks good to me - thanks for the PR @heybran and the review @mukeshpanchal27!

mukeshpanchal27

Thanks @heybran for the changes. Looks good to me.

github-actions · 2025-08-15T19:10:16Z

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 60641
GitHub commit: 8c374a5

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

ajayadav09 · 2025-12-03T21:28:12Z

With the wordpress 6.9 update. it doesn't seem to be fixed. One of the SVG image which has fixed height and width was overridden to have width and heigh of 24 pixels.

i have added this svg image as an icon to the Button component in wordpress component

adamsilverstein · 2025-12-05T17:10:45Z

Hey @ajayadav09 - thanks for the feedback. Would you be able to create a new trac ticket for this so it doesn't get lost? It looks like it might be a separate issue. Also, if possible please share the SVG that is causing the issue.

heybran · 2025-12-06T12:27:31Z

Hi @ajayadav09,

Thanks for the feedback. I don't think this is a bug as the Icon component has a default size of 24 px, which is appearing on your frontend. Although I might be wrong.

The codes for default size of 24px for Icon component can be found here:
https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/icon/index.tsx#L48-L54

ajayadav09 · 2025-12-06T14:02:24Z

@adamsilverstein @heybran thank you for your response and thank you for pointing me to the right direction. Yes I realised the idea to put a custom sized logo in the icon attribute of the button component was not an ideal implementaion, as the icon without assigned width and height is set to 24px. I will close the ticket I had created.

Fix user-provided width and height attribtutes lost on SVG images

bf391aa

Ensure `wp_get_attachment_image()` doesn't overwrite an already valid user-provided width and height.

mukeshpanchal27 suggested changes Jul 24, 2025

View reviewed changes

Update if statement and simplify unit test

c39a862

mukeshpanchal27 approved these changes Jul 25, 2025

View reviewed changes

adamsilverstein requested review from mukeshpanchal27 and removed request for mukeshpanchal27 August 15, 2025 17:35

github-actions bot closed this Aug 15, 2025

	if ( is_numeric( $width ) && ( ! isset( $attr['width'] ) \|\| ! is_numeric( $attr['width'] ) ) ) {
	if ( ( ! isset( $attr['width'] ) \|\| ! is_numeric( $attr['width'] ) && is_numeric( $width ) ) ) {

Conversation

heybran commented Jul 24, 2025

Uh oh!

github-actions bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 24, 2025

Uh oh!

github-actions bot commented Jul 24, 2025

Test using WordPress Playground

Some things to be aware of

Uh oh!

mukeshpanchal27 Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

heybran Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

heybran Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

heybran Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 commented Jul 24, 2025

Uh oh!

mukeshpanchal27 commented Jul 24, 2025

Uh oh!

adamsilverstein commented Jul 25, 2025

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 15, 2025

Uh oh!

ajayadav09 commented Dec 3, 2025

Uh oh!

adamsilverstein commented Dec 5, 2025

Uh oh!

heybran commented Dec 6, 2025

Uh oh!

ajayadav09 commented Dec 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Jul 24, 2025 •

edited

Loading