Cropping site icon should preserve attachment properties

[afercia]

Follow-up to #37750 / [53027]
See #54370 / [57602]

In [53027], a bug was fixed where newly generated cropped images didn't copy over some meta data from the original image like the alt text and caption.

The code that fixed the bug was added in the wp_ajax_crop_image() function but it turns out it was added in the default case of ​the big switch statement in that function.

As such, the meta data are copied over only for the default case, which includes the cropping of the WP logo, but they aren't copied over for the site-icon case.

It could be argued that the site icon image doesn't need the alt text or the caption because it is used in the browsers' UI. However, it would be best to always copy over these data to any cropped image for at least two reasons:

• Cropping an image will generate a new image that is added to the Media Library. It's good that the new image preserves all the data from the original image.
• These meta data are useful in the admin interface. For example now that [57602] adds a site icon UI in the admin General Settings page, the UI needs the alt text of the site icon image to inform users what the currently selected image is. This would be even more important for screen reader users as the current implementation in [57602] doesn't provide any textual information about the currently selected image.

The inconsistency in copying over the meta data can be tested also in the Customizer or in the new UI from [57602].

To reproduce:

• Set Twenty Twenty-One as active theme so that the Customizer is available.
• Go to the Customizer > Site Identity.
• Set a Logo image: make sure to select an image that is large enough to be cropped and that does have an alt text.
• Once the image gets cropped and set as logo, open the Media Library again.
• Observe there's a new cropped image.
• Select the cropped image and observe in the right side of the media modal dialog that the original alt text (and caption if originally present) has been copied to the new image.
• Close the media modal dialog.
• In the Customizer set a Site Icon, repeating the steps above.
• Once the image gets cropped and set as site icon, open the Media Library again.
• Observe there's a new cropped image.
• Select the new image and observe the original alt text (and caption if any) hasn't been copied to the new image.

[afercia]

It's also important to note that some filters, for example wp_ajax_cropped_attachment_id run only for the default case and aren't available for the site-icon case.

Not sure that's ideal. I'd tend to think all the filters within wp_ajax_crop_image should be audited and made available for all cases, when that makes sense.
At the very least, the documentation should clarify that these filters don't run for 'all' the cropped images.

[jorbin]

This could be very prevalent if the alt text is updated depending on the final direction of alt text and UI for #54370, therefore I think it would be good to get a fix in soon.

[rcreators]

I have extended class-wp-site-icon -> create_attachment_object to carry forward parent attachment title, alt text, caption and description as well. This will solve the issue.

[rcreators]

Instead of working on ajax-actions.php->wp_ajax_crop_image function, I worked on class-wp-site-icon.php->create_attachment_object function. I extended create_attachment_object() function to copy original file's meta data, title, desc, caption and alt text and attach it to new cropped image.

[huzaifaalmesbah]

Test Report

Patch tested: 60524.patch​

Environment

• WordPress: 6.5-beta1-57630-src
• PHP: 8.2.15
• Server: nginx/1.25.3
• Database: mysqli (Server: 8.0.36 / Client: mysqlnd 8.2.15)
• Browser: Chrome 121.0.0.0
• OS: macOS
• Theme: Twenty Twenty-One 2.1
• MU Plugins: None activated
• Plugins:
  • Gutenberg 17.7.0
  • Test Reports 1.1.0

Screenshots

Before Apply Patch Crop	After Apply Patch Crop ✅

[joedolson]

Thank you, @rcreators!

This does work, but I don't think that the method is optimal. It does the job, but it splits the logic so that identical logic is being applied in different places. This makes long-term code maintenance harder.

I think that a better path might be to create a helper function that copies the attachment data as needed. E.g.,

function wp_copy_parent_attachment_properties( $attachment, $parent ) {
    // Logic to check and modify attachment array.
    return $attachment; 
}

Then, all the logic is in one place, and we can call it as needed. Thoughts?

[rcreators]

Hello @joedolson,

Yep. Got your point. Do you have any specific file in mind to put this function in? I will do that Today. As it's related to attachment but there is no attachment class or just put in ajax-action file works?

As per the discussion over the ticket, Created the function wp_copy_parent_attachment_properties() in image.php file and called it in ajax-actions.php file.

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

[shailu25]

Test Report

Tested Patch: ​https://github.com/WordPress/wordpress-develop/pull/6128

Testing Environment:

OS: Windows
Web Server: Nginx
PHP: 8.1.23
WordPress: 6.5-beta1
Browser: Chrome
Theme: Twenty Seventeen
Active Plugins: None

Actual Results:

• Issue Resolved With Patch.✅

Screenshots:

Before Patch: ​https://prnt.sc/NEhmi2yk7kgt
After Patch: ​https://prnt.sc/DvUZVXSGFVlV

[rcreators]

@joedolson, PR updated with custom header class as well. It was also cropping the image without copying parent attributes.

About removing the create_attachement_object function from the site icon class, I tried to find it on ​https://wpdirectory.net/, and found some matches but cannot be sure if they are using from the site icon or their own or from somewhere else. So right now we can leave them as it is.

[joedolson]

I think this looks good. However, I think it may be worthwhile to remove the create_attachment_object method. I looked over the directory results, and there are really only two plugins and one theme using this; everything else is people committing vendor directories, build tools, etc. Only one of those plugins has a significant installation base, and I think it's a pretty low bar to change this rather than keep an unused method in core.

But I'd appreciate second opinions on that; it would be removing core methods and breaking backwards compatibility, but only on a very small scale.

I think this is good for commit as is, just would like a second opinion about whether or not to remove the unused method.

[rcreators]

If that's the case, we can remove create_attachment_object method from the site icon class. Removing old code and making WordPress cleaner is also a good thing to proceed further.

[swissspidy]

The create_attachment_object method should be marked deprecated and not directly removed from core. Especially since it is public and used in the ecosystem.

By the way, that method has some unit tests, can this be used as a starting point to add some test coverage for this newly proposed wp_copy_parent_attachment_properties function as well?

Also, just noting that 6.5 RC1 is approaching soon, so this would need to land before that or else we should punt it to 6.6.

[joedolson]

Unit tests added and create_attachment_object deprecated.

[joedolson]

In 57755:

Media: Accessibility: Copy attachment properties on site icon crop.

Add parity between site icon, custom header, and default image crop behaviors. [53027] fixed a bug where alt text and caption were not copied on custom headers, but did not apply that change in any other context.

Deprecate the create_attachment_object method in the Wp_Site_Icon and Custom_Image_Header classes and replace that functionality with the new function wp_copy_parent_attachment_properties() to improve consistency.

Props afercia, rcreators, jorbin, joedolson, huzaifaalmesbah, shailu25, swissspidy, mukesh27.
Fixes #60524.

In r57755

[desrosj]

In 57771:

Coding standards: Apply some changes after composer format.

Follow up to [57565], [57627], [57755],

See #60233, #60506, #60524.