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 #5909 Remove combine CSS option #5911

Merged
merged 14 commits into from Jun 20, 2023

Conversation

CrochetFeve0251
Copy link
Contributor

@CrochetFeve0251 CrochetFeve0251 commented May 9, 2023

Description

Remove the combine CSS feature.
For that we remove the main logic, removed the setting from the page and inside 3rdparties.

Fixes #5909

Type of change

Please delete options that are not relevant.

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

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

No.

How Has This Been Tested?

  • Automated Tests

Checklist:

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 May 9, 2023
@CrochetFeve0251 CrochetFeve0251 added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement module: combine CSS labels May 9, 2023
@CrochetFeve0251 CrochetFeve0251 marked this pull request as ready for review May 9, 2023 10:13
@CrochetFeve0251 CrochetFeve0251 requested a review from a team May 9, 2023 10:14
@Tabrisrp
Copy link
Contributor

There is some conflicts to fix

@vmanthos vmanthos self-requested a review May 16, 2023 10:27
@vmanthos
Copy link
Contributor

@CrochetFeve0251 A minor change.

In the following meta-box, the /combine part should be removed:
image

'minify_css' => __( 'Minify/combine CSS', 'rocket' ),

@vmanthos
Copy link
Contributor

@CrochetFeve0251 When updating to 3.15 (I changed the WP_ROCKET_VERSION of this PR for testing purposes), the cache is cleared but the /cache/min/1/ folder isn't.

wp_rocket_upgrade fired but the on_update() method didn't run.

Can you please look into this?

Steps to reproduce this

  1. Install WP Rocket 3.13.2.
  2. Enable the "Combine CSS files" feature.
  3. Update to 3.15 (that can be done from a custom URL -> doc - internal link ).
  4. Check the /cache/min/1/ folder.

@vmanthos
Copy link
Contributor

@CrochetFeve0251 Thank you for the PR.

The issue from the previous comment is still there. Can you please take another look?

@vmanthos
Copy link
Contributor

vmanthos commented May 30, 2023

@piotrbak @CrochetFeve0251 Currently, the cache, and minified/combined CSS files will be deleted when updating to 3.15 even if the "Combine CSS files" feature was disabled prior to the update.

Is this something we'd like to address?

@piotrbak
Copy link
Contributor

@vmanthos @CrochetFeve0251 Yes, we need to be sure that we'll be able to remove the leftovers later though:

  1. Combine CSS enabled some combined files generated
  2. Turn off Combine CSS
  3. Update the plugin, Combined CSS files are not removed
  4. Clear /min/ directory to remove combined/minified CSS files

If 4 point works okay for the leftovers in this PR, we should handle this. If not, we'll need to leave it like it's.

@vmanthos
Copy link
Contributor

Taking the opportunity from @piotrbak previous comment, I commented out the following line:

'wp_rocket_upgrade' => [ 'on_update', 16, 2 ],

in the 3.15 update file to prevent the cache clearing actions after updating WP Rocket.

Both when Combine CSS files was enabled and when it was disabled before the update the following took place:

  • The cache was cleared /cache/wp-rocket/
  • The minified folder /cache/min/ and its content wasn't cleared

After the update, using "Clear and preload" from the admin bar cleared any leftovers, e.g. previously combined CSS files.

According to Piotr, given this will be a major update, "it will be good to save the resources".

@vmanthos
Copy link
Contributor

vmanthos commented Jun 1, 2023

@CrochetFeve0251 A minor change as far as wording goes.

When Autoptimize's CSS minification is enabled we prevent enabling WP Rocket's CSS minification and we display the following message:

CSS Minification is currently activated in Autoptimize. If you want to use WP Rocket’s minification, disable those options in Autoptimize.

$css_section_helper[] = sprintf( __( '%1$s Minification is currently activated in <strong>Autoptimize</strong>. If you want to use %2$s’s minification, disable those options in Autoptimize.', 'rocket' ), 'CSS', WP_ROCKET_PLUGIN_NAME );

We should change this to:

CSS Minification is currently activated in Autoptimize. If you want to use WP Rocket’s minification, disable the option in Autoptimize.

@CrochetFeve0251 CrochetFeve0251 added this to the 3.15 milestone Jun 5, 2023
@Tabrisrp Tabrisrp changed the base branch from develop to branch-3.15 June 20, 2023 19:08
@Tabrisrp Tabrisrp changed the title Added base from the combine css removal Closes #5909 Remove combine CSS option Jun 20, 2023
@Tabrisrp Tabrisrp merged commit 499e311 into branch-3.15 Jun 20, 2023
9 checks passed
@Tabrisrp Tabrisrp deleted the enhancement/5909-remove-combine-css branch June 20, 2023 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: combine CSS 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.

Get rid of "Combine CSS files" option
4 participants