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

The cached configuration is ignored #1297

Open
webbati opened this issue Jan 23, 2025 · 6 comments
Open

The cached configuration is ignored #1297

webbati opened this issue Jan 23, 2025 · 6 comments
Labels
accepted Issues that have been accepted by the maintainers for inclusion
Milestone

Comments

@webbati
Copy link
Contributor

webbati commented Jan 23, 2025

Winter CMS Build

dev-develop

PHP Version

Other (please specify below)

Database engine

MySQL/MariaDB

Plugins installed

Winter.User, Winter.Blog,Winter.Builder, Winter.Translate

Issue description

Creating a cache file with config:cache command i expect that the new values will be cached and (of course) used by the system.
If i open config.php cached file in storage/framework i can see the correct values but if i don't fill fallback values in the env helper function i'm getting missing values error (for Encription key or database credentials for example)

Running config:clear all come back to works well also if i don't fill the fallback values.

Same behaviour with PHP 8.1.31 and PHP 8.2.16

Steps to replicate

Fill .env file with encription key and database values => All wok's fine

Run config.cache artisan command => You get the errors

Fill fallback values in the env helper function for key value in the app.php file and for database, password and username keys in connections->mysql key in databsse.php file => All work's fine

Remove the falback values that you have just added => You get the errors

Remove cache with config:clear artisan command => All work's fine

Workaround

Fill the fallback values of the env function in the interested files or clear the config cache and don't use it.

@bennothommo
Copy link
Member

bennothommo commented Jan 24, 2025

FYI, this has come up before, but looks like it was auto-archived.
#885

I left some notes on why it is not working here: #885 (comment)

@LukeTowers LukeTowers added the accepted Issues that have been accepted by the maintainers for inclusion label Jan 25, 2025
@LukeTowers LukeTowers added this to the 1.2.8 milestone Jan 25, 2025
@LukeTowers
Copy link
Member

@webbati have you ever noticed any performance improvements from caching the config? Fixing support for it in Winter while retaining support for per-environment configuration overrides is somewhat complicated, so I'd like to make sure it's actually worth doing before we go to the effort of fixing it.

@webbati
Copy link
Contributor Author

webbati commented Jan 26, 2025

@LukeTowers, I thought it was obvious, but I just tested a 10-users simulation with Locust, and the performance seems better without cached configuration. Could I have done something wrong? Is there a better way to test this?

with cache (avg 496ms)
https://i.imghippo.com/files/KyTp3658P.png
https://i.imghippo.com/files/zfKx6003J.png

without cache (avg 357ms)
https://i.imghippo.com/files/peJT2682atI.png
https://i.imghippo.com/files/ieF3896UTo.png

@LukeTowers
Copy link
Member

@webbati Could be issues with our current implementation. Could you test on latest fresh install of just Laravel itself the performance of config:cache on and off? If there's a performance benefit there then potentially it could make sense to investigate in Winter, if not then it's probably overzealous premature optimization and we won't bother fixing it, we'll just remove it.

@webbati
Copy link
Contributor Author

webbati commented Jan 29, 2025

@bennothommo
Copy link
Member

If I could hazard a guess, the reason Laravel changed their config functionality to load everything on initialization was precisely because they wanted to implement caching capabilities, so those benchmarks above don't surprise me.

As mentioned with my comment in the other issue, the way our config is set up makes caching not workable, because we support multiple environments, and we don't know ahead of time all the config keys that will be used since configs can be required by plugins that may be enabled or disabled, and it's possible to define configuration at runtime (such as in a plugin's register or boot methods).

Arguably, we could cache the config as it's read in our current system, but all that would really do is save us a file read if the install is not using file cache. It'll likely be a micro-optimization at best.

I feel our current system is probably the best for what we need it to accomplish, but happy to be proven wrong if someone can come up with a better system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issues that have been accepted by the maintainers for inclusion
Projects
None yet
Development

No branches or pull requests

3 participants