pluginkollektiv / cachify

Smart but efficient cache solution for WordPress. Use DB, HDD, APC or Memcached for storing your blog pages. Make WordPress faster!
https://wordpress.org/plugins/cachify/
GNU General Public License v2.0
99 stars 31 forks source link

Cache doesn't invalidate when permalink changes #285

Closed raffaelj closed 1 year ago

raffaelj commented 1 year ago

Describe the bug

The cache doesn't invalidate when the permalink of a page/post changes.

I use "HDD" mode (which is a bit misleading, because files are stored on SSD) with the proposed changes to .htaccess.

To Reproduce

Expected behavior

System:

Solution

Use post_updated action instead of save_post and check against the original post to invalidate the cache with the original permalink.

While there, internal methods should pass WP_Post instead of int $post_id to save a few db calls.

For reference:

I could send a pull request with changes, but I'm not sure, if this plugin is still actively developed.

Other

I do some research about WP Cache plugins and Cachify looked promising (no bloat, readable code and a rant about CDNs in the docs).

First I tested WP Super Cache, but it loads the whole settings ui on each frontend call, which slows each non-cached request down. Also the code is unreadable.

Cachify is number 2 on my list. I like it, but as mentioned above, I would like to use (and contribute to) a maintained plugin.

Cache Enabler would be the next candidate. I read in an issue here, that it is a fork of Cachify. At first I wanted to drop that because of unhealthy permissions, but I saw, that Cachify has the same issue (chmod with 777 and 666) edit: While skimming over the code I misunderstood the meaning of $stat['mode'] & 0007777.

stklcode commented 1 year ago

Expected behavior

  • When a permalink changes, the cached files /test/index.html and /test/index.html.gz should be deleted.
  • Also the parent folder /test/ should be deleted.

I’d go even further and say more pages or the entire cache should be flushed. The link has changed, so all other pages that point to the modified page (e.g. via menus, where no “page modified“ event has to be triggered explicitly).

But I agree, flushing the cache for the old permalink seems to be a reasonable first action.

raffaelj commented 1 year ago

I’d go even further and say more pages or the entire cache should be flushed.

Good point. But I also use the Redirection plugin, that redirects changed permalinks. So the whole site doesn't break with outdated links. In combination with Cachify, the RewriteRule breaks this behavior:

For general site health and SEO, it's easy to press the cache flush button once in a while. So I would go with:

flushing the cache for the old permalink seems to be a reasonable first action.

raffaelj commented 1 year ago

I wanted to write a fix and immediatly found another bug:

[07-Aug-2023 14:42:42 UTC] PHP Warning:  filemtime(): stat failed for /var/www/html/wp-content/plugins/cachify/css/dashboard.min.css in /var/www/html/wp-content/plugins/cachify/inc/class-cachify.php on line 351
[07-Aug-2023 14:42:42 UTC] PHP Warning:  filemtime(): stat failed for /var/www/html/wp-content/plugins/cachify/css/admin-bar-flush.min.css in /var/www/html/wp-content/plugins/cachify/inc/class-cachify.php on line 359

Copying dashboard.css to dashboard.min.css to admin-bar-flush.css to admin-bar-flush.min.css solved it for now. From a quick look at composer.json and package.json it looks like I would have to install minifycss and minifyjs in the global scope to build everything in the intended way, which I want to avoid.

Are there any other gotchas, I should be aware of when working locally with just throwing the git repo in the plugins folder (actually mounting the git repo as a docker volume)?

stklcode commented 1 year ago

Yep, the minified files are generated in the build process (in Composer post-install or build stage), which requires dev-dependencies.

The project is not really suited for Composer or Git based setup, the tools are just used to generate all files for deployment to the WP repo in the CI/CD pipeline. But I think that’s all. There a a few files in the repo that should not make it into a prod WP instance (basically everything that’s listed in .distignore), but for dev/testing that should be fine.

raffaelj commented 1 year ago

From reading the code it looks like the old version wouldn't invalidate the cache when unpublishing a page. My PR fixes this too. But maybe this is handled in a different hook. I didn't read the whole source.

raffaelj commented 1 year ago

Now I ran composer install to fix the remaining linting errors. composer run-script lint-php doesn't show any errors anymore.

There is one "Code smell" from the SonarCloud tests and I'm not sure, if it's worth fixing.

it looks like I would have to install minifycss and minifyjs in the global scope to build everything in the intended way, which I want to avoid.

I thought, the minify scripts were from npm. But now I realized, that npm modules are only used for linting.

it looks like the old version wouldn't invalidate the cache when unpublishing a page.

Now I tested it with Cachify 2.3.2 and I can confirm, that the cache doesn't invalidate when unpublishing a page (changing status from "Published" to "Private"). It is the same behavior:

Changing the event from save_post to post_updated fixed the unpublishing issue, too.

stklcode commented 1 year ago

Thanks very much for your work. If nobody is faster, I will take a look at your changes later the day.

Resolved the sonar code smell with a comment. Unused intermediate parameters in hook callbacks should be fine.

raffaelj commented 1 year ago

Resolved the sonar code smell with a comment. Unused intermediate parameters in hook callbacks should be fine.

Thanks. I also had another look, if there is a better hook, but $post_before doesn't show up very often in post.php.

After doing some more research and testing a few more plugins I decided to use Cachify (or to write a simple plugin myself, that only supports file caching with apache rewrite rules). So the next question is: When will the next release be published? Or: What's needed to publish a new release?

After my PR is merged, I can install it via wp cli plugin install cachify --version=dev. After looking at the comparison of 2.3.2 and dev branch, using the dev version in production should be save.

edit: It's not save because of an invalid robots.txt.

I found the milestone 2.4.0 with many open issues. My feeling is, that this milestone won't be completed soon. Is there a chance to release a 2.3.3 instead?