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

WIP: When transferring all in-content images, ensure any image URLs in the content are updated to the new image #1283

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Oct 11, 2024

Description of the Change

We currently have two options for processing media when content is distributed:

  1. Only send the featured image
  2. Send the featured image plus any attached media

With the second option turned on, all attached images (if using the Classic Editor) or any block image (if using the Block Editor) will be transferred to the destination site. But we don't actually update any of the in-content URLs, so those will still point to the origin site.

This PR fixes that when the second option is turned on. We keep track of any images we distribute and we then parse through the content of the newly created item to find any old image URLs and update with the new image URLs. This is set up to work both for the Block Editor and Classic Editor.

Note

We try and determine the original image size and use that when getting the new image URL. If an image size from the origin site doesn't exist on the destination site, the full size image will be used instead.

For the Classic Editor

We parse through the entire post content, using the HTML API, to find all images. We pull out the image classes and use those to match the existing image using the image ID class and the image size using the image size class. If no classes exist or the image ID class doesn't exist, we skip that image since we can't be sure it's a match. Note we could change this logic to try matching based on the image URL but the problem here is we have the original full image URL which is likely not the same URL used in the content (as that's typically a resized image URL).

For any images found that match the image ID, we use the HTML API to update the image src with the image that matches the image size class and update the image ID class.

For the Block Editor

We parse the content into blocks and find any image blocks that have the old image ID as a block attribute (similar to above, we could match based on URL but for image blocks, I think we can depend on the image ID always existing and being easier to match on). We update the ID block attribute and then use the HTML API to update the image src with the proper sized image (based on the image size block attribute) and update the image class.

Note that right now we only update image blocks. We could look to add support for other blocks that use images, like the cover block, media and text block, etc.

Closes #1278, #1262,

How to test the Change

  1. Ensure the Media Handling setting at wp-admin/admin.php?page=distributor-settings is set to Process the featured image and any attached images
  2. Create a new Block Editor post and add at least one image block. Add new images to those blocks (this ensures the image is attached to the post)
  3. Push this content to a connection
  4. View the content on the destination site and ensure the image URLs reflect that site
  5. Run through the same steps but install the Classic Editor plugin on both sites first and create a new classic post
  6. Run through all of the above again but Pull content instead of Push
  7. If desired, run through the same steps but use an external connection instead of an internal connection

Changelog Entry

Added - When the setting to distribute all images is turned on, do a search/replace on the distributed content to update the image URLs from the origin site to the destination site

Credits

Props @dkotter, @willemb2, @dcarrionc, @lucymtc

Checklist:

@dkotter dkotter added this to the 2.1.0 milestone Oct 11, 2024
@dkotter dkotter self-assigned this Oct 11, 2024
@dkotter dkotter requested a review from a team as a code owner October 11, 2024 21:26
@dkotter dkotter requested review from peterwilsoncc and removed request for a team October 11, 2024 21:26
@github-actions github-actions bot added the needs:code-review This requires code review. label Oct 11, 2024
@dkotter
Copy link
Collaborator Author

dkotter commented Oct 11, 2024

Things I still see as todo's here:

  • Add E2E tests for this new image handling
  • Decide if we want a new setting for this or keep it tied to the existing Process the featured image and any attached images setting
  • Determine if it's worth more effort around determining the proper sized image to use. Right now we try and determine the image size that was used and get that. If that image size isn't found, we use the full image instead. We could try to temporarily register the proper image size, resize the image and use the resulting URL if we think that's worth it
  • Thorough testing, especially with external connections, posts with lots of images, posts with a mix of existing and newly uploaded images (so some images that aren't actually attached), sites with different image sizes (both different dimensions of the default image sizes and custom image sizes that only exist on the origin site)

Comment on lines +1131 to +1149
if ( $processor->next_tag( 'img' ) ) {
$src = wp_get_attachment_image_url( $image_id, $image_size );

// If the image size can't be found, try to get the full size.
if ( ! $src ) {
$src = wp_get_attachment_image_url( $image_id, 'full' );

// If we still don't have an image, skip this block.
if ( ! $src ) {
continue;
}
}

$processor->set_attribute( 'src', $src );
$processor->add_class( 'wp-image-' . $image_id );
$processor->remove_class( 'wp-image-' . $media_item['id'] );

$blocks[ $key ]['innerHTML'] = $processor->get_updated_html();
$blocks[ $key ]['innerContent'][0] = $processor->get_updated_html();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would love feedback on if there's a better approach here. I was hoping I could just update the block attributes and then re-render the block to have it rebuild the saved block markup but nothing I tried actually worked, so I ended up with this approach of basically doing a search/replace from the old URL to the new URL

Copy link
Member

Choose a reason for hiding this comment

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

I tried using render_block and serialize_block to avoid directly assigning to innerHTML, but that broke the image block due to inconsistency between the content returned by save() and edit().

@dkotter dkotter linked an issue Oct 11, 2024 that may be closed by this pull request
1 task
@Sidsector9
Copy link
Member

Sidsector9 commented Oct 23, 2024

@dkotter I was testing out this PR and found an image duplicating issue.

When a post is pulled, images are duplicated in the media library when:

  • The pulled post contains the same image multiple times, or
  • If multiple posts that are pulled use the same image

@dkotter
Copy link
Collaborator Author

dkotter commented Oct 23, 2024

When a post is pulled, images are duplicated in the media library

Hmm.. interesting. This PR doesn't change how images are imported, just updates the actual in-content image URLs. Could you test those same scenarios on the latest release to see if you see the same thing?

@Sidsector9
Copy link
Member

@dkotter yep, its the same with 2.0.6 and the develop branch. Should I raise a separate issue?

@dkotter
Copy link
Collaborator Author

dkotter commented Oct 23, 2024

@dkotter yep, its the same with 2.0.6 and the develop branch. Should I raise a separate issue?

Yeah, if there's not already another issue about this, we should open one

@Sidsector9
Copy link
Member

@dkotter yep, its the same with 2.0.6 and the develop branch. Should I raise a separate issue?

Yeah, if there's not already another issue about this, we should open one

We have an issue for this - #765

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
2 participants