Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename attachment images to be supported by Subsizes #426

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

iuravic
Copy link
Collaborator

@iuravic iuravic commented Feb 1, 2024

This PR

Subsizes does not allow attachment filenames to end in -\d+x\d+, e.g. image-100x200.jpg. If you upload a file like that to Atomic, it will be accepted, but a -1 will be appended to its filename and attachment post object's post_name, e.g. you will get image-100x200-1.jpg uploaded to your media library. If we're cloning historic WP sites to Atomic, such images won't work once Subsizes starts appending its size suffixes.

This PR fixes those attachments by:

  • looping through all attachment objects and find which end in -\d+x\d+ suffix
  • renames those files and attachment objects
  • updates usage of those attachments in post_content

Caveat!

This needs to be run before conversion to blocks. Because conversion to blocks might append an additional -\d+x\d+ on top of the existing old -\d+x\d+. E.g. invalid image in HTML Dorothy-336x225.jpg might be updated to Dorothy-336x225-336x225.jpg by Gutenberg. So it's important to run this before converting to blocks.

How to test

  • create a site on your Local, and upload some local images with suffixes, like image-100x200.jpg
  • use these images in posts
  • run the command
  • verify that image-100x200.jpg has been renamed to image-100x200-1.jpg both in Media Library's URL to image, and in posts where this image was used

  • confirmed that PHPCS has been run

@iuravic iuravic requested a review from a team February 1, 2024 21:11
@iuravic iuravic self-assigned this Feb 1, 2024
Copy link
Member

@naxoc naxoc left a comment

Choose a reason for hiding this comment

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

This looks great – if the wp_unique_filename won't work for this feel free to merge it.

The nit is not important, but I keep seeing the instance var in our code and it confuses me 😆

Generally I think we should use PHP 8 return declarations on functions now that we have left PHP 7 behind, so I'd love to also see them. (feel free to also ignore that one).


// Pt. 1. Find unique filename for attachment by appending "-\d+" to filename. Subsizes does not allow filenames ending in "-\d+x\d+", and it appends a -\d to the filename, so we must do the same.
$max_suffixes = 1000;
$new_filename = $this->find_unique_filename( $filename, $max_suffixes );
Copy link
Member

Choose a reason for hiding this comment

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

*
* @return InterfaceCommand|null
*/
public static function get_instance() {
Copy link
Member

Choose a reason for hiding this comment

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

This is a nit, but there is no reason to keep an instance var for the simpleton, so this would be simpler:

Suggested change
public static function get_instance() {
public static function get_instance(): self {
static $instance = null;
if ( null === $instance ) {
$instance = new self();
}
return $instance;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

@iuravic
Copy link
Collaborator Author

iuravic commented Feb 2, 2024

Generally I think we should use PHP 8 return declarations on functions now that we have left PHP 7 behind, so I'd love to also see them. (feel free to also ignore that one).

Agreed, I think we're ready to add this to our PHPCS standard. Feel free to if you'd like.

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