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

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

Conversation

thijsoo
Copy link
Contributor

@thijsoo thijsoo commented Aug 14, 2024

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 full-sized counterpart when a resized first content image is used for Open Graph and X images.

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:

Note: The improvement doesn't work if indexables haven't been built, we will publish a relevant note in the docs about this.

  • 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 note that now we also output the og:image:width|height|type tags, so make sure they are correct.
  • 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 right og:image tag.
  • Now repeat the tests after setting Enable media pages to true in Yoast settings and resetting the indexables.

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

  • 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.
We also moved the getting info from images to its own class which was introduced here: #20380. So these steps should be repeated as a regression test.

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

  • 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.
  • 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 thijsoo added the changelog: enhancement Needs to be included in the 'Enhancements' category in the changelog label Aug 14, 2024
…image-tags-are-not-showing-the-biggest-available-image-when-a-resized-version-is-used-in-the-content
Copy link
Contributor

@leonidasmi leonidasmi left a comment

Choose a reason for hiding this comment

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

With this PR, we introduce parsing the post content in the post indexable builder, which means that we now do the heavy lifting of searching for images whenever we try to build a post indexable.

I have some fundamental issues with the performance of this, let me try to articulate them below, to have them in one place:

  • If a post hasn't had its indexable built yet, accessing that post in the frontend will take much more time than before, due to us trying to search for image IDs on page load.
    • Which also means that, if a site has disabled the indexable creation (eg. a staging site), every time that a post is visited the post content is going to be searched again
  • If indexables are not yet build (eg. in a site that just had Yoast installed) and you visit the All posts page in the admin, our new flow will search the content of a lot of posts in a single request, which is bound to face performance issues in the wild, especially if there are a couple of long posts there.
  • On post creation, the content is going to be searched for images twice, once for this new feature and one for the old feature of link building. (This can be rectified using cache, so we do the heavy lifting once per request)

What makes things worse is that the addition of all this performance overheard might be in vain, as we know that this new image search is not compatible with a lot of cases (eg. the classic editor's and Elementor's galleries).

@leonidasmi
Copy link
Contributor

🐛 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
Copy link
Contributor

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).

@coveralls
Copy link

coveralls commented Aug 22, 2024

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%
Files with Coverage Reduction New Missed Lines %
src/generated/container.php 1030 0.39%
Totals Coverage Status
Change from base Build 708a25c44c91c0b546a81f39a7d6c1d84735c376: -0.01%
Covered Lines: 29753
Relevant Lines: 54626

💛 - Coveralls

Copy link
Contributor

@leonidasmi leonidasmi left a comment

Choose a reason for hiding this comment

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

CR: 🚧

src/helpers/image-helper.php Outdated Show resolved Hide resolved
src/helpers/image-helper.php Outdated Show resolved Hide resolved
src/helpers/image-helper.php Show resolved Hide resolved
src/builders/indexable-link-builder.php Show resolved Hide resolved
src/builders/indexable-link-builder.php Show resolved Hide resolved
src/builders/indexable-link-builder.php Show resolved Hide resolved
src/builders/indexable-link-builder.php Outdated Show resolved Hide resolved
Copy link
Contributor

@leonidasmi leonidasmi left a comment

Choose a reason for hiding this comment

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

Acceptance: 🟠 (Looks good, a couple of minor things below)

3 notes:

Note 1:
As per the comment below, I think we need an additional check to prevent empty IDs from being stored in the indexable database. When that's done, another quick smoke test round should happen and we should be good to merge.

Note 2:
We are now outputting the og:image:width|height|type tags for when a first content image is used for OG images, whereas we didn't before. I assume it's fine because it's how we do when a featured image is selected for OG image, but best to check with product/SEO team, for explicit approval.

Note 3:
Now, when a first content image is used for the OG/Twitter images, an ID is stored in the indexable. But that ID is not stored in the post meta, where we would expect to find it in the _yoast_wpseo_opengraph-image-id, which is what happens eg. when we set an OG image in the Social tab. I didn't find any regressions due to that and it already was the same case when we used the feature image for the OG image, so I'm mentioning this just for future reference and I think it's worth a double check from you @thijsoo .

src/builders/indexable-link-builder.php Outdated Show resolved Hide resolved
…image-tags-are-not-showing-the-biggest-available-image-when-a-resized-version-is-used-in-the-content
@leonidasmi leonidasmi added this to the 23.6 milestone Sep 13, 2024
@leonidasmi leonidasmi merged commit 5aac91f into trunk Sep 13, 2024
23 checks passed
@leonidasmi leonidasmi deleted the 19849-og-image-tags-are-not-showing-the-biggest-available-image-when-a-resized-version-is-used-in-the-content branch September 13, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Needs to be included in the 'Enhancements' category in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OG image tags are not showing the biggest available image when a resized version is used in the content
3 participants