keycdn / cache-enabler

A lightweight caching plugin for WordPress that makes your website faster by generating static HTML files.
https://wordpress.org/plugins/cache-enabler/
123 stars 46 forks source link

Cache settings location #135

Closed nlemoine closed 4 years ago

nlemoine commented 4 years ago

Hi @coreykn

I just updated cache-enabler and noticed that the location of setting file has changed.

May I ask why? I looked for answers in commits and issues but couldn't really find any reasons.

I'm using bedrock/composer and deployer to deploy my projects and the new location doesn't play well with those environments.

Although there's no official guideline on that topic, IMHO, storing setting files inside the plugin folder is a bad practice.

I guess this is too late now 😢

coreykn commented 4 years ago

This was done in #92 because we don't believe settings files should be stored in a cache directory. This directory is for cached content, which a settings file is not. It makes sense to me to store our files in our plugin folder.

Can you please let me know why it's a bad practice in addition to more details about the issue you're now experiencing with this change?

If you or someone else has a better suggestion I'm more than happy to consider it.

futtta commented 4 years ago

maybe storing in /wp-content/ or /wp-content/uploads/ would work better?

On Fri, Sep 18, 2020 at 7:34 PM Corey notifications@github.com wrote:

This was done in #92 https://github.com/keycdn/cache-enabler/pull/92 because we don't believe settings files should be stored in a cache directory. This directory is for cached content, which a settings file is not. It makes sense to me to store our files in our plugin folder.

Can you please let me know why it's a bad practice in addition to more details about the issue you're now experiencing with this change?

If you or someone else has a better suggestion I'm more than happy to consider it.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/keycdn/cache-enabler/issues/135#issuecomment-694994785, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABMIMM54O6MMO7NZYAVKHDSGOKZ3ANCNFSM4RRZXSLQ .

nlemoine commented 4 years ago

Thanks for your feedback @coreykn.

I'll try to explain why I consider this a bad practice. This is necessarily opiniated, it doesn't mean it's right 😉

From a design perpective, in modern WordPress development, plugins are considered as "packages" and handled by composer. They are like the "vendor" directory in modern PHP applications. This means they can be removed and reinstalled safely whithout losing settings and that data should not be located there.

As I wrote in my first comment, there's no official standard about not storing things in your plugin folder. You can of course disagree but I think it's a quite shared concept in the community (Bedrock, WPStarter, WP Cubi and more stacks out there).

composer plugins management or modern development aside, this practice seems also shared among some plugin developers. I don't know any plugins that stores data in their plugin folder. I can't obviously tell for every plugin because there are so many but WP Rocket, for example, stores its config in WP_CONTENT_DIR . '/wp-rocket-config'. WP Super Cache has a similar behavior.

From a more personal point of view, I use deployer to deploy my projects which creates releases (e.g. a new folder) each time you deploy. To persist some files/folders across deployments, you have to declare which ones will be shared. Usually WP_CONTENT_DIR/uploads and the .env file (or wp-config.php in a traditional setup). Everything else is installed (with composer) on each deployement. I added your settings folder to the shared folders. But in the deployment process, composer install happens after the creation of symlinks to the shared folders. I had to execute some tasks twice as a workaround:

➤ Executing task deploy:update_code
✔ Ok
➤ Executing task deploy:shared
✔ Ok
➤ Executing task deploy:writable
✔ Ok
➤ Executing task deploy:vendors
✔ Ok
➤ Executing task deploy:shared
✔ Ok
➤ Executing task deploy:writable
✔ Ok

It works but it's not ideal. In a more secure environment (the project I'm currently working on has a stricter permissions policy than usual), I also have to tell which folders are writable. Make a plugin folder writbale feels not right to me.

Anyway, even you forget my personal setup issue, I think the design arguments should be strong enough. I totally understand that storing settings file in the cache folder didn't feel right to you. But moving it to the plugin folder seems worse to me. Storing it in a WP_CONTENT_DIR/cache-settings or any other folder name inside WP_CONTENT_DIR would probably be better?

As a side note, storing settings as a PHP file like WPRocket would probably be faster.

coreykn commented 4 years ago

Thank you both for your feedback, it's sincerely appreciated. I'll see what can be done to improve this.

nlemoine commented 4 years ago

Thanks for considering this!