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 #4868 Apply CDN to used CSS on output instead of on saving #4893

Merged
merged 9 commits into from
Apr 28, 2022
Merged

Fixes #4868 Apply CDN to used CSS on output instead of on saving #4893

merged 9 commits into from
Apr 28, 2022

Conversation

CrochetFeve0251
Copy link
Contributor

@CrochetFeve0251 CrochetFeve0251 commented Apr 5, 2022 •
edited
Loading

Description

Solves a problem where CDN exclusions are not taken in consideration when using RUCSS.
To solve this problem the filter rocket_usedcss_content has been moved from check_job_status to get_used_css_markup

Fixes #4868

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

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

No

How Has This Been Tested?

  • Automated tests

Please delete the options that are not relevant.

  • 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 self-assigned this Apr 5, 2022
@CrochetFeve0251 CrochetFeve0251 added type: bug Indicates an unexpected problem or unintended behavior module: CDN labels Apr 5, 2022
@remyperona remyperona changed the title Moved the filter and removed the clean_used_css_with_cdn method Fixes #4868 Apply CDN to used CSS on output instead of on saving Apr 11, 2022
@remyperona
Copy link
Contributor

One thing to fix in the test for PHP 8

TypeError: error_reporting(): Argument #1 ($error_level) must be of type ?int, string given

/tests/Unit/inc/Engine/Optimization/RUCSS/Controller/UsedCSS/treeshake.php:31

@remyperona remyperona added this to the 3.11.1 milestone Apr 11, 2022
@CrochetFeve0251
Copy link
Contributor Author

@Tabrisrp this is the PR where we got the problem with mockery. That function call is here to prevent the error to raise but we agreed with @engahmeds3ed that wasn't clean.

@remyperona
Copy link
Contributor

If this is the solution for now it's fine, but you are passing a string to error_reporting and it expects int value, so removing the quotes should be enough to prevent this error from showing on PHP 8

Mai-Saad
Mai-Saad approved these changes Apr 12, 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

@remyperona
Copy link
Contributor

I'd suggest to skip the test for now, as even with the updated test suite it's still not passing. The issue is coming from the dependency, so we can't really change anything at that time.

@engahmeds3ed engahmeds3ed merged commit 9f68883 into develop Apr 28, 2022
@engahmeds3ed engahmeds3ed deleted the fix/4868-move-filter-usedcss-content branch April 28, 2022 13:50
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

Assignees

@CrochetFeve0251 CrochetFeve0251

Labels
module: CDN type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Milestone
3.11.1
Development

Successfully merging this pull request may close these issues.

CDN exclusions are not reflected in Used CSS
4 participants
@CrochetFeve0251 @remyperona @Mai-Saad @engahmeds3ed