How should environmental settings be configured by default? #11865
Replies: 16 comments 33 replies
-
|
Not a vote, but my opinion is that either way is fine, as long as I can put it into version control. |
Beta Was this translation helpful? Give feedback.
-
|
Honestly @brandonkelly if you can do: That would give the best of both worlds in my honest opinion. |
Beta Was this translation helpful? Give feedback.
-
|
I think the example given in Option 1 isn't a great one, because looks complicated to people, with a whole bunch of conditionals. It also looks unfamiliar to people who have been using the craftcms/craft starter for some time. If what you're really trying to decide is whether in the default starter you should have it leveraging the I know I'd choose option #2 (assuming I didn't know about the other issues with the Option 1: $isDev = App::env('CRAFT_ENVIRONMENT') === 'dev';
$isProd = App::env('CRAFT_ENVIRONMENT') === 'production';
return GeneralConfig::create()
->defaultWeekStartDay(1)
->omitScriptNameInUrls()
->devMode($isDev)
->allowAdminChanges($isDev)
->disallowRobots(!$isProd)
;Option 2: // config/general.php
return GeneralConfig::create()
->defaultWeekStartDay(1)
->omitScriptNameInUrls();# .env
CRAFT_DEV_MODE=true
CRAFT_ALLOW_ADMIN_CHANGES=true
CRAFT_DISALLOW_ROBOTS=1 |
Beta Was this translation helpful? Give feedback.
-
|
@khalwat This refactor of Option 1 is much cleaner and easier to read. 👍 |
Beta Was this translation helpful? Give feedback.
-
|
@khalwat option 1 expressed like this gets my vote: easy to explain and reason about, explicit, flexible, transparent. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah I mean that's my point... that's what they are already using here: https://github.com/craftcms/craft/blob/main/config/general.php (minus the comments) If they want a meaningful vote on whether to go with |
Beta Was this translation helpful? Give feedback.
-
|
As a way to fiddle with the fluent flat config file approach, I just updated a production site today to $isDev = App::env('ENVIRONMENT') === 'dev';
$isProd = App::env('ENVIRONMENT') === 'production';
return GeneralConfig::create()
->defaultWeekStartDay(1)
->omitScriptNameInUrls()
->limitAutoSlugsToAscii(true)
->defaultSearchTermOptions([
'subLeft' => true,
'subRight' => true,
])
->devMode($isDev)
->allowAdminChanges($isDev)
->disallowRobots(!$isProd)
->securityKey(App::env('SECURITY_KEY'))
->cpTrigger(App::env('CP_TRIGGER') ?: 'admin')
->aliases([
'@web' => App::env('BASE_URL'),
'@baseUrl' => App::env('BASE_URL'),
'@basePath' => App::env('BASE_PATH'),
'@assetsBasePath' => App::env('BASE_PATH').'/uploads',
'@assetsBaseUrl' => App::env('BASE_URL').'/uploads',
'@webroot' => App::env('BASE_PATH')
])
;Which is pretty explicit and readable (that autocomplete / doc for config variables is a very nice addition). Wrote a similar |
Beta Was this translation helpful? Give feedback.
-
|
I'm changing my vote - initially I liked the idea of keeping the current config - but the more I think about it the more I think this is the only file that defines things for multiple environments. The rest of the Craft ecosystem only defines the environment you are working in currently. I think for the sake of consistency and understanding it's actually probably better to handle configuration at the .env level. |
Beta Was this translation helpful? Give feedback.
-
|
Currently, config files with simple logic and lean
If we are talking about a default for new projects, my vote would go to a few When teaching to newbies at school, I feel a solid config file with simple logic + a minimal amount of |
Beta Was this translation helpful? Give feedback.
-
That’s exactly the type of config settingns in question here. Per the second example, we would continue recommending using |
Beta Was this translation helpful? Give feedback.
-
|
I am mainly with @khalwat here. This is a front-end dev perspective, so maybe take it with a grain of salt
To sum things up for default craft installs:
|
Beta Was this translation helpful? Give feedback.
-
|
Please don't go all environment variables. I discuss why in detail here: https://nystudio107.com/blog/fluent-multi-environment-config-for-craft-cms-4#config-settings-via-environment-variable Environment variables were originally intended for local development only, but everyone started using them in production anyway. Environment variables are intended to be for things that vary from environment to environment. Putting every setting into environment variables seems like shoehorning settings into a place where they were never intended to be. More importantly, option 2 splits the config between two separate files, so you have to hunt through both to figure out where a setting is being set. One file invisibly overrides the other, which is not great from a transparency POV. I can easily envision a situation where someone is explicitly setting There would be no indication that the setting is being overridden via the By having the default project working this way, you're setting people up to have issues, imo. Also with any change, you have to consider the benefit of the change vs. the cost of it. I'm not seeing the benefit outweighing the cost here. I strongly agree with @michtio's comment. We can already do this: // config/general.php
$config = GeneralConfig::create()
->defaultWeekStartDay(1)
->omitScriptNameInUrls()
->devMode(App::env(DEV_MODE))
->allowAdminChanges(App::env(ALLOW_ADMIN_CHANGES)
->disallowRobots(App::env(DISALLOW_ROBOTS);
return $config;Not seeing the benefit of either option 1 and especially not option 2 The best solution here is changing nothing, imo |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for the extended answer. |
Beta Was this translation helpful? Give feedback.
-
|
I agree, this is by far the best approach. It's the best of all worlds, easiest to read & easiest to manage. |
Beta Was this translation helpful? Give feedback.
-
That’s not remotely suggested. We will definitely continue defining things that don’t need to change per-environment in
Not sure where you’re getting that from? They’ve always been needed for storing API keys, etc., at the very least.
Fair, and mentioned in the Cons for option 2. |
Beta Was this translation helpful? Give feedback.
-
Well they are already there as magic hidden overrides to config settings, which I think is problematic for reasons discussed below (see my replies to @lindseydiloreto) And option 2 encourages that behavior. I'd be happy to see the magic
As |
Beta Was this translation helpful? Give feedback.
-
I’m not sure I agree with that statement.. following things like 12factor apps and deploying to containers… everything is defined at an environment level or secret (depending upon the platform). If someone messed up a production config and disabled robots changing an environment variable to override that makes a lot of sense to me.. especially if the company is large and broken into multiple teams… an ops guy doesn’t want to touch code and redeploy to potentially break production, but in most platforms it’s easy to change an environment to toggle something on and then off again to change the behavior of the app. Ultimately it depends on the context, single person shop vs agency vs large team/enterprise. It actually feels like command line flag parsing in CLI applications… the existence of the flag overrides the environment variable. |
Beta Was this translation helpful? Give feedback.
-
The phpdotenv developer had it in all caps in his README.md for years not to use it in production, which is how I suspect most people are using Obviously, it's different in terms of "using it on production" if we're talking about injecting the variables into the environment via Docker container or via Nginx, or what have you. As to "everything" being defined at an environment level, I'm just not sure I love the idea of all 158 config settings for Craft CMS (not to mention the DB settings) being defined via It's also a whole lot more error-prone to do it that way, you have to convert the setting from the Craft setting into SCREAMING_SNAKE_CASE and you get no autocomplete, documentation, or type checking when doing so (which was introduced by the fluent config) |
Beta Was this translation helpful? Give feedback.
-
|
Oh, yeah your post wasn't clear.. I did not mean using an actual .env file.. I mean the platforms environment variable system (ECS, Kubernetes, Cloud Run, and etc). No one should be using a .env in production.. I also agree all settings should not be defined in the .env but have the capability to override using an environment variable if that makes sense? The defaults for a config should be sensible.. Does that make more sense? |
Beta Was this translation helpful? Give feedback.
-
Agreed; but I'd wager that a majority of Craft CMS sites are in fact using
It does, but I am not a fan for the transparency scenario mentioned above (re-pasted here): I can easily envision a situation where someone is explicitly setting There would be no indication that the setting is being overridden via the By having the default project working this way, you're setting people up to have issues, imo. |
Beta Was this translation helpful? Give feedback.
-
Short answer — Either method is probably fine, for the specific configs you listed. In my projects, I define these three configs in the PHP config, based on a single env var: I specifically prefer to put these in the PHP config, because:
(Notably, the Long answer — Even though it'd probably be fine to treat some configs as exceptional overrides, it's still preferable not to: The You've hit the nail on the head here:
...but I draw slightly broader boundaries around what "default" means. To me, a default config is anything that I expect to be present across multiple environments. For example, it may be true that the value of my default site URL changes from env to env... but I still consider this part of my default setup because every env has/needs this value. The benefits of version control, a single mental source-of-truth, and fluent IDE auto-completion, make leaning on PHP configs my preference for the vast majority of configs. (And, I don't want to clutter up my All that said, I can envision use cases where reading config directly from the
So, it's definitely worth raising awareness that this method exists, but only in special cases. Once a config value — whether static or environment-specific — is expected or shared in multiple envs, I typically consider it to be worthy of committing to the PHP config file. (And I think this preference is worthy of elevating as a best-practice, especially in Admittedly, my preference is biased by living in agency-world for most of my career, where I highly prize being able to (a) infer the likely config of a site at a glance, from the code, and (b) easily onboard new devs, especially novices, to lots of different projects. I find it's more practical to optimize for human understanding. From that standpoint, the cons of Option 2 outweigh the cons of Option 1, if we're talking about establishing a default convention that most sites are going to use out of the box. |
Beta Was this translation helpful? Give feedback.
-
|
@michaelrog wrote:
Agree with all of these scenarios (and that they are exceptional), but you can still get all of that with the current config by doing this: // config/general.php
$config = GeneralConfig::create()
->defaultWeekStartDay(1)
->omitScriptNameInUrls()
->devMode(App::env(DEV_MODE))
->allowAdminChanges(App::env(ALLOW_ADMIN_CHANGES)
->disallowRobots(App::env(DISALLOW_ROBOTS);
return $config;You can then still tinker with the environment variable settings to your heart's content, and it'll all work. The key difference being it is not an invisible override of the config settings. It's a transparent one. I'd honestly be in favor of eliminating the If you need to do that, put the setting into an environment variable that the |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, the key is whether you expect to be setting a config ahead of time (so you write it into your config file and deploy it), vs tinkering at runtime. I like being able to inject runtime overrides, and I think the |
Beta Was this translation helpful? Give feedback.
-
|
I hear ya. I think that's an edge case of an edge case tho, most likely. And I think it's better to be intentional about it when you do, rather than magic overrides. |
Beta Was this translation helpful? Give feedback.
-
|
Are you really going to introduce a "magic" prefixed That seems pretty extreme, especially considering that the list could grow or change over time. It made much more sense to have magic environmental variables for the database configuration. There are only about 8-10 database settings (which is probably all you'll ever need), and DB settings are highly environment-dependent. I just don't think you can say "this helped with |
Beta Was this translation helpful? Give feedback.
-
It's already there. #10573 |
Beta Was this translation helpful? Give feedback.
-
|
Yikes, I totally missed that. I don't like it (with the exception of DB values), so I guess I just won't use it. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah I'm not a fan either. Here's what it does: Line 115 in ac616d4 It iterates over every property that So even if you don't use it at all it's checking every property every time. Mostly I'm not a fan of the lack of transparency this introduces (scenarios mentioned in above posts) |
Beta Was this translation helpful? Give feedback.
-
When this was implemented, I did some fairly extensive performance profiling and the overhead was barely measurable. Performance should not be a concern here. |
Beta Was this translation helpful? Give feedback.
-
|
Yes, I saw the performance profiling, and noted that while it is 20x slower than without it, we're talking nanoseconds. So it's not an issue. I wasn't mentioning performance, but rather that you have no choice, it does this for all of the settings regardless of whether you use it at all, or want to use it. |
Beta Was this translation helpful? Give feedback.
-
|
To throw my hat in the ring: I think the config override feature should be considered just that: a way to override config, without having to commit or deploy code - i.e., being able to toggle something like Even if we're currently mostly talking about config settings that are environmental in nature (like |
Beta Was this translation helpful? Give feedback.
-
|
I fully agree. I'd like to be able to look in the If someone only sets a |
Beta Was this translation helpful? Give feedback.
-
|
Right, or – migrating the project to a new server, and missing a crucial config setting in the new production environment because nobody knew it was even set in the old one. |
Beta Was this translation helpful? Give feedback.
-
|
I agree, though I'll go even further to say that I think So if I have in my config file: it's possible for in the Being explicit and intentional in the And you can still override the |
Beta Was this translation helpful? Give feedback.
-
|
Even just reading this thread is giving me gray hair. 😬 |
Beta Was this translation helpful? Give feedback.
-
|
There are clearly a lot of strong opinions in both directions here, which is understandable, as I think it is really personal preference. I think it really boils down to:
Personally, I love the magic env (admittedly biased) – as I often support Craft apps I didn't build, and I want to be able to set something without combing through a |
Beta Was this translation helpful? Give feedback.
-
|
Config overrides is potentially a massively useful feature and I can think of tons of use cases for them. There are some big caveats involved though and they feel like a power user kind of thing to me – I just don't know if they make the most sense for a default config in the starter project; something that is probably used as the boilerplate for most Craft projects by developers at all different kinds of levels. This feature is also available regardless of what the project's config file actually looks like. If the concern is that the ability to configure Craft with |
Beta Was this translation helpful? Give feedback.
-
|
I'm equally comfortable doing devops and doing Craft development. But I think most Craft developers are better served by clear, transparent ways of configuring Craft. Screaming snake case magic env vars I think have too many potential gotchas, and too many downsides compared to relatively niche devops benefits. The primary benefit I see from them is being able to override a Config setting that is not in the general.php file pulling from an environment variable. Most of the time, the env vars you might want to change on the fly are already environment-specific, and thus would already be coming from and environment variable via general.php The relatively rare times that this isn't the case I don't think outweighs all of the negatives that come along with using them, for the majority of Craft developers |
Beta Was this translation helpful? Give feedback.
-
|
Ooops, I take it back -- I forgot that I asked to have this changed/fixed in craftcms/craft@e84b53e |
Beta Was this translation helpful? Give feedback.
-
|
I believe this statement by @timkelty (emphasis mine)...
... and this statement by @mmikkel (emphasis mine)...
... are in complete and total agreement. They are both talking about the But for your typical day-to-day Craft usage, it seems excessive. So let's go back to the original question (emphasis mine)...
My conclusion...Designed as a tool for power users in extraordinary circumstances, the With the power of EXCEPTION - Database ConfigFor all that I said above regarding the general config settings, I don't believe the same logic applies to database config settings. The DB configuration settings are a different scenario with very different considerations...
Despite what I said about the general config, it does make sense to use |
Beta Was this translation helpful? Give feedback.
-
|
Agree with pretty much everything above regarding
|
Beta Was this translation helpful? Give feedback.
-
|
Personally I don't like the magic As a side note, here is a solution that would weaken the first con point of option 1:
Laravel recently introduced a new
|
Beta Was this translation helpful? Give feedback.
-
|
I liked it! Two points here: 1 - A Utility & CLI command that would show you all of the config settings that are different from the default, their values (redacted as needed), and critically where they are being set from (which config file or which 2 - I've been hoping for something similar to Laravel's |
Beta Was this translation helpful? Give feedback.
-
|
Reading all these comments here with good points, pros and cons and thinking about final decision that @brandonkelly and his team behind Craft CMS must make must be frustrating. I think I get it that @brandonkelly wants Craft CMS to be great UX for developers, content creators etc. and best CMS in the world but I'd always look at it from performance point of view even if a difference is bearable. ... plus more settings come in a future. No matter how luxurious for developer spinning up a new Craft CMS's install would be, we build stuff for end user (visitor of a website or an app). Craft CMS should be fast and ecological in processing a request. Doing one more round just to check if there are some more settings and those settings precedes other settings is kind of Personally I wouldn't go for overriding settings with Although I would like Based on what the option 2. would do under the hood I choose performance over developer's comfort and therefore |
Beta Was this translation helpful? Give feedback.
-
|
Thanks to everyone for chiming in! At the end of the day, this comment (and others with a similar sentiment) won me over, for ruling out option 2:
We decided to go with @michtio’s hybrid approach (list out each of the settings in use in From config/general.php: return GeneralConfig::create()
// Set the default week start day for date pickers (0 = Sunday, 1 = Monday, etc.)
->defaultWeekStartDay(1)
// Prevent generated URLs from including "index.php"
->omitScriptNameInUrls()
// Enable Dev Mode (see https://craftcms.com/guides/what-dev-mode-does)
->devMode(App::env('DEV_MODE') ?? false)
// Allow administrative changes
->allowAdminChanges(App::env('ALLOW_ADMIN_CHANGES') ?? false)
// Disallow robots
->disallowRobots(App::env('DISALLOW_ROBOTS') ?? false)
;We‘ve also replaced the one
|
Beta Was this translation helpful? Give feedback.
-
We are debating how environmental settings like
devMode,allowAdminChanges, anddisallowRobotsshould be configured by default, in thecraftcms/craftstarter project.Option 1: From the config file
The config file checks the
CRAFT_ENVIRONMENTenvironment variable, and sets the config setting values accordingly. (This is close to how it currently works.)Pros:
config/general.php.Cons:
config/general.php.Option 2: With environment variables
config/general.phpis only responsible for defining the default config, leaving all environment-specific changes up to each environment, via environment variables.# .env CRAFT_DEV_MODE=true CRAFT_ALLOW_ADMIN_CHANGES=true CRAFT_DISALLOW_ROBOTS=1Pros:
Cons:
50 votes ·
Beta Was this translation helpful? Give feedback.
All reactions