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

Add a new backend exclusion field for Incompatible Plugins #6041

Closed
piotrbak opened this issue Jul 12, 2023 · 4 comments · Fixed by #6065
Closed

Add a new backend exclusion field for Incompatible Plugins #6041

piotrbak opened this issue Jul 12, 2023 · 4 comments · Fixed by #6065
Assignees
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [M] 3-5 days of estimated development time module: dynamic lists type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@piotrbak
Copy link
Contributor

piotrbak commented Jul 12, 2023

Is your feature request related to a problem? Please describe.
We'll add new array to be handled by the backend, it's related to incompatible plugins:

function rocket_plugins_to_deactivate() {

Additional context
The backend part will be handled here:
#6041

Acceptance Criteria:

  1. Incompatible plugins feature should work without regressions using the backend list, notices of incompatible plugins should be displayed
@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting needs: grooming module: dynamic lists labels Jul 12, 2023
@mostafa-hisham mostafa-hisham added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. and removed needs: grooming labels Jul 17, 2023
@mostafa-hisham
Copy link
Contributor

Identify the root cause

  • To avoid editing the code each time we want to add a new Incompatible plugin we need to add a new configuration for them in the backend.
  • but it is more complicated than other exclusions lists we use because it has conditions based on what options are enabled in the plugin

Scope a solution ✅

backend side

  • create new pages for IncompatiblePlugins similar to the one we have in delayJS (list, add, edit, delete).
  • fields for IncompatiblePlugins should be title [required], slug [required], condition, and file [required].
  • the condition can be empty so the plugin will be incompatible by default and don't have to activate a certain option.
  • the condition can also contain || so we can use it as or condition to handle plugins with or conditions like these.

plugin side

  • we need to separate it in a new JSON file as we did in delay js.
  • create a new folder with an APIClient and DataManager IncompatiblePlugins in Engine/Optimization/DynamicLists.
  • update the providers list in Optimization/DynamicLists/ServiceProvider to add the new one for Incompatible plugins.
  • update GitHub workflow to download the new file.
  • remove the static array here and use the data from backend to create the same plugins array.
  • use this filter rocket_plugins_to_deactivate in Optimization/DynamicLists/Subscriber.php to inject the new array from backend data.
  • we need to implement a meet_conditions function to handle data from the backend with or || in the condition field
    something like this.
 function meet_conditions( $conditions = "" ) {
    if ( empty( $conditions ) ) {
        return true;
    }

    $conditions = explode( '|', $conditions );

    foreach( $conditions as $condition ) {
        if ( get_rocket_option( $condition, false ) ) {
            return true
        }
    }

    return false;
}
  • update and create tests.

Estimate the effort ✅

[M]

@engahmeds3ed please check the grooming especially the plugin part in case I missed anything and thanks for the meet_conditions.

@mostafa-hisham mostafa-hisham added effort: [M] 3-5 days of estimated development time and removed GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels Jul 17, 2023
@MathieuLamiot
Copy link
Contributor

Thanks for the detailed grooming @mostafa-hisham !

It could be useful to detail a bit the acceptance criteria:

Acceptance Criteria:

Incompatible plugins feature should work without regressions using the backend list, notices of incompatible plugins should be displayed

Should we check all plugins one by one ? Sounds tedious.
What do you think of:


  • On a site with WP Rocket and without incompatible plugins
    • no warning is displayed.

Testing without conditions

  • Install w3-total-cache
    • a warning must be displayed
  • Install combine-css
    • a warning must be displayed
  • uninstall w3-total-cache
    • a warning must be displayed
  • uninstall combine-css
    • no warning must be displayed

### Testing with one condition

  • Deactivate lazyload
  • Install bj-lazy-load
    • No warning must be displayed
  • Activate lazyload
    • A warning must be displayed

### Testing with two conditions

  • Deactivate minify_css and minify_js
  • Install wp-super-minify
    • No warning must be displayed
  • Activate minify_css
    • A warning must be displayed
  • Activate minify_js
  • A warning must be displayed
  • Deactivate minify_css
  • A warning must be displayed

Can a software engineer perform those tests? It might be sufficient, without additional help from QA?
(cc @Mai-Saad, @vmanthos )

@piotrbak
Copy link
Contributor Author

@MathieuLamiot

**Testing without conditions**
Install w3-total-cache
-a warning must be displayed
Install combine-css
-a warning must be displayed
uninstall w3-total-cache
-a warning must be displayed
uninstall combine-css
-no warning must be displayed

I'm not sure if I understand the above part, let's discuss it. I believe that e2e tests for the above scenario would be more than enough, no?

You're right about the AC, that's my bad.

@vmanthos
Copy link
Contributor

@MathieuLamiot I agree with @piotrbak that e2e testing should be sufficient for the testing without conditions.

For the rest, where e2e may be more complicated to implement it's either QA or devs - based on availability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [M] 3-5 days of estimated development time module: dynamic lists type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants