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 #6033 Add partial clean of SpinupWP cache #6100

Merged
merged 3 commits into from
Sep 18, 2023

Conversation

Tabrisrp
Copy link
Contributor

Description

Add partial clean of SpinUpWP cache on our clean actions

Fixes #6033

Type of change

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

How Has This Been Tested?

  • Manual test

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • 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

@Tabrisrp Tabrisrp self-assigned this Aug 10, 2023
@Tabrisrp Tabrisrp added 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting type: enhancement Improvements that slightly enhance existing functionality and are fast to implement labels Aug 10, 2023
@Tabrisrp Tabrisrp marked this pull request as ready for review August 11, 2023 12:58
@Tabrisrp Tabrisrp requested a review from a team August 11, 2023 12:58
@piotrbak
Copy link
Contributor

Access to the website and SFTP is on Bitwarden.

@Mai-Saad Mai-Saad self-requested a review September 6, 2023 12:24
@Mai-Saad
Copy link

Mai-Saad commented Sep 7, 2023

@Tabrisrp Thanks for the PR. During exploratory testing, can see that Purge this URL from the Admin bar at page view or clear this cache from the posts list is not working (i.e not setting cache header to Miss) when permalinks without / i.e /%postname% However, it is working when permalinks with/ i.e /%postname%/. can you please check 🙏
Tested env: https://spinupwp.rocketlabsqa.ovh/
Active plugins
Screenshot from 2023-09-18 09-39-35

@Tabrisrp
Copy link
Contributor Author

Tabrisrp commented Sep 8, 2023

I updated the code to add a trailing slash to the URL to purge

@Mai-Saad
Copy link

@Tabrisrp Thanks for the update. can still see the problem on the test site using the latest version of the PR. can you please check 🙏

@Tabrisrp
Copy link
Contributor Author

I can't find the reason why it's not working without trailingslash in the code itself. There is nothing that should impact it, looking at their code: https://github.com/spinupwp/spinupwp-plugin/blob/62ae3ec7c53f602f80cd10c742fae6c361c790f4/src/Cache.php#L289

We might need to ask them if there is a reason on their side

@MathieuLamiot
Copy link
Contributor

@Mai-Saad, @piotrbak, would it make sense to close the issue as is and open a new one to identify the issue with trailing slash? As this probably needs discussion with Spinup and/or R&D?

@piotrbak, do you have a contact at Spinup we could reach out to?

@Mai-Saad
Copy link

@MathieuLamiot I am fine with that, can execute the test plan only while Permalinks having / for now

@piotrbak
Copy link
Contributor

@MathieuLamiot @Tabrisrp @Mai-Saad Agreed, let's do it like that. As soon as the issue is opened, I'll reach SpinupWP and ask them to collaborate there

@MathieuLamiot
Copy link
Contributor

MathieuLamiot commented Sep 15, 2023

Ok, @Tabrisrp can you open an issue? I think you have more insights on the matter :) Moving this to "Ready for QA" in case there are other things to check :)
Thanks!

@Mai-Saad
Copy link

Mai-Saad commented Sep 18, 2023

Working as expected with the following notes.

  • on the trunk with spinupWP (preload off)=> it seems they don't differentiate mobile from desktop
    1- Enable separate mobile cache
    2- clear cache from admin
    3- visit the desktop of the page => cache miss
    4- visit mobile of same page => cache hit
    => As per discussion, SpinupWP does not provide this option
  • edit post => clear whole cache (same on trunk and even without wpr)
  • switch theme => not clearing home hit header ( UI is correct but the cache is hit, same on the trunk but without wpr cache will be miss in this case), it will cause UI issue even on the trunk while RUCSS is on due to Used CSS isnot automatically generated for home page when switch theme while preload is off #6076 (comment)
    testrail-report-525.pdf

@Mai-Saad Mai-Saad added this pull request to the merge queue Sep 18, 2023
Merged via the queue into develop with commit 9ef6d1e Sep 18, 2023
8 checks passed
@Mai-Saad Mai-Saad deleted the enhancement/6033-spinupwp branch September 18, 2023 11:55
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 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.

Compatibility with SpinupWP cache purging (indiviual URLs)
6 participants