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

UHF-10406: Catch exceptions from UrlResolverInterface::getProviderByUrl #884

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

hyrsky
Copy link
Member

@hyrsky hyrsky commented Jan 10, 2025

UHF-10406

What was done

Fix crashes with some YouTube link.

Sentry issue: https://sentry.hel.fi/organizations/city-of-helsinki/issues/46100/?query=is%3Aunresolved+OEmbed&referrer=issue-stream&statsPeriod=14d

This issue was hard to reproduce locally. I believe the root cause was that google blocks OpenShift environment:
image

Some YouTube links, like https://youtube.com/watch?v=g2eYKMjE8ew (missing www.), are perfecly fine, but not included in the list provider by drupal/oembed_providers: https://oembed.com/providers.json. This causes these links to make network requests to google from the backend whenever they are used. According to the Sentry logs, Google seems to be banning our production environment.

These PRs include two fixes to this issue:

  • Handle the exception when thrown.
  • Add better sanitization video URLs in Rekry migrations, where these links have mostly come from, so no network requests to Google are needed in the first place.

How to install

  • Make sure your Rekry is up and running on latest dev branch.
    • git pull origin UHF-10406
    • make fresh
  • Update the Helfi Platform config
    • composer require drupal/helfi_platform_config:dev-UHF-10406
    • composer require drupal/hdbt:dev-UHF-10406
  • Run make drush-updb drush-cr

How to test

  • Verify that exceptions no longer crash the page. Simulate google being angry at us by adding throw new ResourceException('Test.', $url); to the top of UrlResolver::getProviderByUrl. View some pages that have video embeds. They should not be shown at all (unless cached).
  • Verify that rekry migrations remove videos that not present in Helbit. Add video to some job listing, run: drush migrate:import helfi_rekry_jobs --reset-threshold 1 --update. The video should be removed.
  • Check that code follows our standards

Continuous documentation

  • This feature has been documented/the documentation has been updated
  • This change doesn't require updates to the documentation

Translations

  • Translations have been added to .po -files and included in this PR

Other PRs

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.77%. Comparing base (983a87e) to head (df07e70).

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #884   +/-   ##
=========================================
  Coverage     17.77%   17.77%           
  Complexity      285      285           
=========================================
  Files            36       36           
  Lines          1080     1080           
=========================================
  Hits            192      192           
  Misses          888      888           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hyrsky hyrsky marked this pull request as ready for review January 10, 2025 07:36
Copy link
Contributor

@annadruid annadruid left a comment

Choose a reason for hiding this comment

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

Works as described!

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.

3 participants