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

Closes #6312: Exclude youtube thumbnail from lazyload #6391

Merged

Conversation

jeawhanlee
Copy link
Contributor

@jeawhanlee jeawhanlee commented Jan 18, 2024

Description

This PR excludes youtube thumbnail from lazyload

Fixes #6312

Type of change

  • Enhancement (non-breaking change which improves an existing functionality).

Is the solution different from the one proposed during the grooming?

No

Checklists

Generic development checklist

  • My code follows the style guidelines of this project, with adapted comments and without new warnings.
  • I have added unit and integration tests that prove my fix is effective or that my feature works.
  • The CI passes locally with my changes (including unit tests, integration tests, linter).
  • Any dependent changes have been merged and published in downstream modules.
  • If applicable, I have made corresponding changes to the documentation. Provide a link to the documentation.

Test summary

  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I validated all Acceptance Criteria of the related issues. (If applicable, provide proof).
  • I validated all test plan the QA Review asked me to.

@jeawhanlee jeawhanlee added the type: enhancement Improvements that slightly enhance existing functionality and are fast to implement label Jan 18, 2024
@jeawhanlee jeawhanlee self-assigned this Jan 18, 2024
@jeawhanlee jeawhanlee requested a review from a team January 18, 2024 11:21
@jeawhanlee
Copy link
Contributor Author

For CR, I Backported PR from wp-media/rocket-lazyload-common#116

@jeawhanlee jeawhanlee marked this pull request as ready for review January 18, 2024 11:25
@Mai-Saad
Copy link
Contributor

Mai-Saad commented Jan 19, 2024

@jeawhanlee Thanks for the PR. Please find exploratory test notes below.
1- When tried to exclude https://i.ytimg.com/vi/8bUSQjuNedI/hqdefault.jpg from UI on this page https://new.rocketlabsqa.ovh/video-testing/ (LL image , LL video and preview image are already enabled), the image wasnot excluded from LL
2- If we set the filter to return Null, we will have this warning
[19-Jan-2024 15:13:01 UTC] PHP Warning: foreach() argument must be of type array|object, null given in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Dependencies/RocketLazyload/Iframe.php on line 252
Can you please check 🙏

@jeawhanlee
Copy link
Contributor Author

When tried to exclude https://i.ytimg.com/vi/8bUSQjuNedI/hqdefault.jpg from UI on this page https://new.rocketlabsqa.ovh/video-testing/ (LL image , LL video and preview image are already enabled), the image wasnot excluded from LL

@Mai-Saad I was checking this but if we support exclusion from the UI, it would exclude the YT iframe from being lazyloaded and we don't want that.
@piotrbak Do we add another UI for this or just leave this as a filter as it is.

@piotrbak
Copy link
Contributor

@jeawhanlee Just to be sure, if we'd add i.ytimg.com/vi/8bUSQjuNedI/hqdefault.jpg to the UI we'd exclude the iframe? 🤔

Iframe doesn't have hqdefault.jpg in the source

@remyperona
Copy link
Contributor

That exclusion wouldn't do anything. At the time we can manage the exclusion, the thumbnail is not created yet in the page (it's dynamically generated with JS, based on the YT ID).

@jeawhanlee jeawhanlee requested a review from remyperona January 26, 2024 18:33
@Mai-Saad
Copy link
Contributor

Mai-Saad commented Feb 8, 2024

@jeawhanlee Thanks for the update. Now we can exclude thumbnail image using UI or filter. However, we need to guard the filter from invalid values as now if set to null, we will have this error. can you please check 🙏

[08-Feb-2024 17:54:42 UTC] PHP Fatal error:  Uncaught TypeError: WP_Rocket\Engine\Media\Lazyload\Subscriber::add_exclusions(): Argument #1 ($exclusions) must be of type array, null given, called in /var/www/newer.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php on line 324 and defined in /var/www/newer.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/Lazyload/Subscriber.php:459
Stack trace:
#0 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): WP_Rocket\Engine\Media\Lazyload\Subscriber->add_exclusions()
#1 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(205): WP_Hook->apply_filters()
#2 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Dependencies/RocketLazyload/Assets.php(275): apply_filters()
#3 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Dependencies/RocketLazyload/Assets.php(209): WP_Rocket\Dependencies\RocketLazyload\Assets->getYoutubeThumbnailScript()
#4 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/Lazyload/Subscriber.php(254): WP_Rocket\Dependencies\RocketLazyload\Assets->insertYoutubeThumbnailScript()
#5 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): WP_Rocket\Engine\Media\Lazyload\Subscriber->insert_youtube_thumbnail_script()
#6 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters()
#7 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action()
#8 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-includes/general-template.php(3068): do_action()
#9 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-content/themes/twentytwentyone/footer.php(78): wp_footer()
#10 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-includes/template.php(790): require_once('...')
#11 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-includes/template.php(725): load_template()
#12 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-includes/general-template.php(92): locate_template()
#13 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-content/themes/twentytwentyone/archive.php(38): get_footer()
#14 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-includes/template-loader.php(106): include('...')
#15 /var/www/newer.rocketlabsqa.ovh/htdocs/wp-blog-header.php(19): require_once('...')
#16 /var/www/newer.rocketlabsqa.ovh/htdocs/index.php(17): require('...')
#17 {main}
  thrown in /var/www/newer.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/Lazyload/Subscriber.php on line 459

@Mai-Saad
Copy link
Contributor

@hanna-meda
Copy link
Contributor

Relating issue found while testing this: #6451

Copy link
Contributor

@hanna-meda hanna-meda left a comment

Choose a reason for hiding this comment

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

@hanna-meda hanna-meda added this pull request to the merge queue Feb 21, 2024
Merged via the queue into develop with commit 96cd8a0 Feb 21, 2024
9 checks passed
@hanna-meda hanna-meda deleted the enhancement/6312-exclude-youtube-thumbnail-from-lazyload branch February 21, 2024 11:51
@piotrbak piotrbak added this to the 3.15.10 milestone Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: lazyload iframes and videos module: lazyload images module: lazyload youtube preview image type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It's not possible to exclude the YouTube thumbnail from LazyLoad
5 participants