-
-
Notifications
You must be signed in to change notification settings - Fork 651
Improve Craft CMS "settings management" (writing to project .env
)
#7233
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
Comments
I would really like to see us use .ddev/.env. Note that environment variables defined in It seems like just creating/managing |
Yep, sounds like a plan @rfay |
@timkelty we'll need at least your reaction and followup on
|
@rfay On my list today! |
While it's not required, it's used by the template https://github.com/craftcms/craft, and
I opened: From Discord https://discord.com/channels/664580571770388500/1372597483443589264: Using I'm still getting the A new install message when I run |
Just adding in here as general feedback so it's not lost in Discord. This change is something I just don't want in our projects, and all it's done in introduce a new thing I've had to add into the project's root level .gitignore file, and a level of confusion about what's actually happening. Context: I was working on our long-standing starter project, from which other web projects are built, and the new behaviour added the I appreciate this may be a little niche, but as a general thought - I was under the impression that .env anything should not be in GIT, because the purpose is that these are environmental data that changes, and shouldn't be in Git because those values differ based on where you're actually running the project. Frankly, for us, we don't want any of the stuff DDEV adds into |
Thank you for the feedback @MattWilcox.
It can be disabled completely: ddev config --disable-settings-management |
As you see @MattWilcox this was requested by @timkelty of Pixel & Tonic. We generally only do explicit CMS integrations in collaboration with the communities involved, and this one was done over time with collaboration from the Craft CMS folks. |
We just tagged new Tested installing Craft 4 and 5 on an older version of DDEV as well as v1.24.6, and all works as expected. Older versions will append the
@MattWilcox That’s true for the root |
It's also worth noting that |
@timkelty @brandonkelly I'm figuring out how to address those changes in our existing projects and our Craft boilerplate. Removing the I found a much better solution, in my opinion, is to switch from This might be a good default for Correct me if I'm missing something … @MattWilcox I agree with you that even the env files in /.ddev/.env
/.ddev/.env.* |
@MoritzLost We used to use We just discussed whether it would make sense to switch back in light of this DDEV change, but decided to keep it as-is, as the (now) current behavior is more in line with expected behavior at first glance. (We expect it would confuse people to have environment variables defined in That said, if you’d prefer to stick with a single |
@brandonkelly Good to know the history, thanks! Yeah, it might be confusing for new devs if their We'll stick with using I've added a note at the top of our
|
Uh oh!
There was an error while loading. Please reload this page.
Is there an existing issue for this?
Is your feature request related to a problem?
The Craft CMS project type writes a number of
CRAFT_*
vars to the project's.env
file, which is not ideal for a few reasons:vlucas/phpdotenv
CRAFT_DB_
vars, but often times don't realizeCRAFT_WEB_ROOT
was added by DDEV, leave it in place, and break their remote app.Describe your solution
The way things are implemented now seems to be somewhat by chance and a fundamental misunderstanding of how the "settings management" feature is intended to be used.
Ideally, DDEV wouldn't touch the project's
.env
file, and instead inject the environment variables directly into the containers.Describe alternatives
It appears that writing
.ddev/.env
is one option for that, but writing to YAML may also be an option.Additional context
The text was updated successfully, but these errors were encountered: