wordpress-seo icon indicating copy to clipboard operation
wordpress-seo copied to clipboard

19849 og image tags are not showing the biggest available image when a resized version is used in the content

Open thijsoo opened this issue 1 year ago • 3 comments

Context

  • We want to make sure we detect the first content image as best as possible. By checking for the ID we can make sure to get the original image size instead of just the URL.

Summary

This PR can be summarized in the following changelog entry:

  • Uses the correct og image size when the first content image is used in posts edited in the block editor.

Relevant technical choices:

  • We chose to not update the open_graph_image and twitter_image field to the non resized one, since making sure that something is resized and then getting the correct image url would be error prone due to needing a Regex and it would be a hit to the performance of saving an post.
  • We also now rely on the image ID instead of the image URL making all og and twitter image sources rely on the ID

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

  • Create a new post in the block editor and add 1 image in the content. Select the thumbnail size for this image.
  • Publish the post and make sure the full image url is used in the "og:image" tag.
  • Also make sure that in the indexable for this post the twitter_image_id and open_graph_image_id are filled in.
  • Change the first image to a different image and make sure the first content image is updated.
  • Now add a featured image and make sure that image is used instead of the first content image.
  • Smoke test that classic and elementor also still add the first content image to the indexables and show the og:image tag.

Relevant test scenarios

  • [ ] Changes should be tested with the browser console open
  • [ ] Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • [ ] Changes should be tested on different editors (Default Block/Gutenberg/Classic/Elementor/other)
  • [ ] Changes should be tested on different browsers
  • [ ] Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • [X] QA should use the same steps as above.

QA can test this PR by following these steps:

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

Since this PR touches the core way the first content image is set. All ways of setting the OG and Twitter images should be smoke tested, and it needs to be made sure that the priorities still work as expected.

UI changes

  • [ ] This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

  • [ ] This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

  • [ ] I have written documentation for this change. For example, comments in the Relevant technical choices, comments in the code, documentation on Confluence / shared Google Drive / Yoast developer portal, or other.

Quality assurance

  • [X] I have tested this code to the best of my abilities.
  • [ ] During testing, I had activated all plugins that Yoast SEO provides integrations for.
  • [ ] I have added unit tests to verify the code works as intended.
  • [ ] If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
  • [ ] I have written this PR in accordance with my team's definition of done.
  • [X] I have checked that the base branch is correctly set.

Innovation

  • [ X] No innovation project is applicable for this PR.
  • [ ] This PR falls under an innovation project. I have attached the innovation label.
  • [ ] I have added my hours to the WBSO document.

Fixes #

thijsoo avatar Aug 14 '24 11:08 thijsoo

🐛 A direct effect of us introducing this content parsing is that we now get OOM errors because of an infinite loop, whenever there's the Yoast breadcrumbs block in a post content ⚠️

leonidasmi avatar Aug 16 '24 09:08 leonidasmi

One possible alternative approach could be:

  • Whenever the links of a post are updated and we detect that the first content image is a resized one, we check whether the OG image source is first-content-image
    • this can be done in the Indexable_Link_Builder::update_related_indexables() method, by which point in time we have already parsed the post content for image IDs.
  • if it is, we know we have a resized image in the OG image URL
  • so we now can replace the OG image URL with the original image URL using the image ID we already have in our hands

The advantage of this approach is that we keep the heavy lifting of searching for image IDs where it originally was (on the post creation or the SEOO) so we don't add performance overhead then.

Worth to note that the performance overhead of this approach is negligible, because the extra calculations described above don't require extra db read, since everything we need is already in our disposal (including the OG image source).

leonidasmi avatar Aug 16 '24 09:08 leonidasmi

Pull Request Test Coverage Report for Build 9ff9ba3bbe5b6fba43f1cf478a9c991578c14c24

Details

  • 56 of 74 (75.68%) changed or added relevant lines in 3 files are covered.
  • 1030 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 54.713%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/images/Application/image-content-extractor.php 44 52 84.62%
src/builders/indexable-link-builder.php 8 18 44.44%
<!-- Total: 56 74
Files with Coverage Reduction New Missed Lines %
src/generated/container.php 1030 0.39%
<!-- Total: 1030
Totals Coverage Status
Change from base Build 708a25c44c91c0b546a81f39a7d6c1d84735c376: -0.01%
Covered Lines: 29753
Relevant Lines: 54626

💛 - Coveralls

coveralls avatar Aug 22 '24 12:08 coveralls