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.

Sign up for GitHub

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

Jump to bottom

Fixes #4759 Hide scripts from the buffer #4838

Merged
merged 10 commits into from
Apr 11, 2022
Merged

Fixes #4759 Hide scripts from the buffer #4838

merged 10 commits into from
Apr 11, 2022

Conversation

CrochetFeve0251
Copy link
Contributor

@CrochetFeve0251 CrochetFeve0251 commented Mar 22, 2022 •
edited by remyperona
Loading

Description

Fix a bug where images inside scripts were having dimensions too breaking the JS script.
Centralized hide functions in RegexTrait.

Fixes #4759
Fixes #4767

Type of change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which improves an existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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

No the grooming was right

How Has This Been Tested?

  • Automated tests
  • Local tests

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@CrochetFeve0251 CrochetFeve0251 added type: bug Indicates an unexpected problem or unintended behavior module: media labels Mar 22, 2022
@CrochetFeve0251 CrochetFeve0251 self-assigned this Mar 22, 2022
@CrochetFeve0251 CrochetFeve0251 requested review from engahmeds3ed and a team March 22, 2022 09:25
@remyperona remyperona changed the title Fix/4759 hide scripts from the buffer Fixes #4759 Hide scripts from the buffer Mar 22, 2022
@remyperona remyperona added this to the 3.11.1 milestone Mar 28, 2022
@Mai-Saad
Copy link
Contributor

@CrochetFeve0251 @Tabrisrp please find exploratory test notes below:

  • Can see this fatal error with LL images on the test template, can you please check it.
29-Mar-2022 12:00:54 UTC] PHP Fatal error:  Uncaught TypeError: preg_match_all() expects parameter 2 to be string, null given in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Dependencies/RocketLazyload/Image.php:29
Stack trace:
#0 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Dependencies/RocketLazyload/Image.php(29): preg_match_all('#<img(?<atts>\\s...', NULL, NULL, 2)
#1 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/Lazyload/Subscriber.php(378): WP_Rocket\Dependencies\RocketLazyload\Image->lazyloadImages('\n<!DOCTYPE html...', '\n<!DOCTYPE html...', false)
#2 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(307): WP_Rocket\Engine\Media\Lazyload\Subscriber->lazyload('\n<!DOCTYPE html...')
#3 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(189): WP_Hook->apply_filters('\n<!DOCTYPE html...', Array)
#4 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/classes/Buffer/class-optimization.php(104): apply_filters('rocket_buff in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Dependencies/RocketLazyload/Image.php on line 29
  • @piotrbak LL image is applied on commented/uncommented NoScript and on commented <img, is this ok, or do we need GH for this?
  • Missing dimension is added to commented images<img, is this expected with the latest change?

@CrochetFeve0251
Copy link
Contributor Author

@Tabrisrp should I use the RegexTrait also in Image?

@piotrbak
Copy link
Contributor

🤔 We should not apply lazy load markup to the images that are inside the <noscript></noscript>. Our library is based on JS so it won't work when JS is not available

@remyperona
Copy link
Contributor

We can hide the comments in lazyload and image dimensions too

Mai-Saad
Mai-Saad approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@Mai-Saad Mai-Saad left a comment

Choose a reason for hiding this comment

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

Working as expected with the following notes:

  • LL images and add missing dimensions are applied on commented <img
  • CDN is applied to URLs in comments
  • Delay js is applied to commented scripts
    testrail-report-366.pdf

@remyperona remyperona merged commit bfbe206 into develop Apr 11, 2022
@remyperona remyperona deleted the fix/4759-hide-scripts-from-the-buffer branch April 11, 2022 17:23
CrochetFeve0251 added a commit that referenced this pull request May 2, 2022
@engahmeds3ed @CrochetFeve0251
@remyperona @Klemart3D @jeawhanlee @DahmaniAdame @vmanthos
3.11.1 (#4983)
3a5b9b1 
* Fixes #4757 Exclude AMP redirection script from delay JS (#4827)

* Added amp-mobile-version-switcher to the HTML DelayJs exclusion array

* Added amp-mobile-version-switcher script the delayJs fixture to ensure by automated testing that this script is not deleted

* Reverted changes in DelayJS and fixtures corresponding to it.

* Added a call to the filter rocket_delay_js_exclusions in the AMP Subscriber to exclude amp-mobile-version-switcher

* Wrapped the filter rocket_delay_js_exclusions in the AMP verification to launch it only when it is activated

* Fixed code standards

* Added integration test for the exclude_script_from_delay_js method

* Update version in inc/ThirdParty/Plugins/Optimization/AMP.php

Co-authored-by: Rémy Perona <[email protected]>

* Use configTestData instead of providerTestData tests/Integration/inc/ThirdParty/Plugins/Optimization/AMP/excludeScriptFromDelayJs.php

Co-authored-by: Rémy Perona <[email protected]>

* Remove providerTestData function in tests/Integration/inc/ThirdParty/Plugins/Optimization/AMP/excludeScriptFromDelayJs.php

Co-authored-by: Rémy Perona <[email protected]>

* Removed providerTestData mehod in tests/Unit/inc/ThirdParty/Plugins/Optimization/AMP/excludeScriptFromDelayJs.php

Co-authored-by: Rémy Perona <[email protected]>

* Use configTestData instead of providerTestData in tests/Unit/inc/ThirdParty/Plugins/Optimization/AMP/excludeScriptFromDelayJs.php

Co-authored-by: Rémy Perona <[email protected]>

* Update inc/ThirdParty/Plugins/Optimization/AMP.php

Co-authored-by: Rémy Perona <[email protected]>

* Update tests/Unit/inc/ThirdParty/Plugins/Optimization/AMP/excludeScriptFromDelayJs.php

Co-authored-by: Rémy Perona <[email protected]>

* Update tests/Integration/inc/ThirdParty/Plugins/Optimization/AMP/excludeScriptFromDelayJs.php

Co-authored-by: Rémy Perona <[email protected]>

* Added Integration TestCase

Co-authored-by: Rémy Perona <[email protected]>

* Fixes #4804 Use absint values for width/height values when setting image dimensions of SVG files (#4805)

* Fixes #4527 Remove Rocket option to deactivate lazyload of iframes and YT videos when Autoptimize lazyload is enabled (#4889)

* Closes #4020 Delete metadata from the postmeta table during uninstall (#4855)

* Closes #4133 Clean feeds only if our cache feeds helper plugin is enabled (#4833)

* Fixes #4171 Exclude judge.me script from delay JS (#4839)

* Fixes #4032 Disabling heartbeat causing dependency error in query monitor (#4888)

* Prevent issues when parsing the HTML to apply optimizations (#4838)

* Fixes #4758 Exclude our remove CPCSS script from delay JS (#4824)

* Closes #4896 Update test suite (#4903)

Update our test suite to be cross-compatible with WP and PHPUnit versions

* Prevent PHP 8.1 deprecation notices (#4732)

* Flatsome preserver dynamic #image_ style (PR #4979)

Fixes #4971

* Preserve Health Center dynamic CSS selectors (PR #4976)

* Closes #4867 Update the deactivation form UI (PR #4880)

* Remove render class
* move deactivation form assets to deactivation intent class
* update classes instantiation
* add subscriber class
* update code for new deactivation intent form
* delete new option and transient on uninstall
* remove modal task
* remove old modal CSS
* remove old modal JS
* use micromodal for deactivation modal
* update the form HTML
* update the modal CSS
* update micromodal to v0.4.10
* return type hint
* add transition
* replace divs by list
* remove unneeded comment
* add missing semicolon
* update activate_safe_mode tests
* add unit tests
* add fixtures
* cast option to int
* update used hooks
* remove screen check
* update test and fixture
* integration tests
* fix PHPCS

* Fixes #4868 Apply CDN to used CSS on output instead of on saving (PR #4893)

* Moved the filter and removed the clean_used_css_with_cdn method
* Removed fixtures and integration tests from the method `clean_used_css_with_cdn`
* Added unit tests for treeshake function from UsedCSS controller
* Fixed code standards
* Removed quotes
* Fixed errors with the tests
* replicate magic isset declaration from parent class
* revert previous commit

Co-authored-by: Rémy Perona <[email protected]>

* Fix problem with action scheduler (PR #4965)

* Added fix
* Added fix standards
* Added check on pending
* Fixed search
* Added fix standards
* Fixed to conform to CR
* Fixed code standards
* Fixed code standards
* Changed `search` method usage to keep backward compatibility
* stop bailing out after cancel all actions

Co-authored-by: Ahmed Saeed <[email protected]>

* update plugin version

* Fixed failing test (#4986)

Co-authored-by: COQUARD Cyrille <[email protected]>
Co-authored-by: Rémy Perona <[email protected]>
Co-authored-by: Klemart3D <[email protected]>
Co-authored-by: michael lee <[email protected]>
Co-authored-by: Rémy Perona <[email protected]>
Co-authored-by: Adame Dahmani <[email protected]>
Co-authored-by: Vasilis Manthos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mai-Saad Mai-Saad Mai-Saad approved these changes

@remyperona remyperona remyperona approved these changes

@engahmeds3ed engahmeds3ed Awaiting requested review from engahmeds3ed

Assignees

@CrochetFeve0251 CrochetFeve0251

Labels
module: media type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Milestone
3.11.1
4 participants
@CrochetFeve0251 @Mai-Saad @piotrbak @remyperona