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

Ignore web/sites/*/settings.php #178

Merged
merged 2 commits into from
Feb 14, 2017
Merged

Conversation

ceefour
Copy link
Contributor

@ceefour ceefour commented Jul 17, 2016

No description provided.

@leymannx
Copy link
Collaborator

leymannx commented Oct 12, 2016

Maybe web/sites/*/settings.local.php can be ignored in advance as well?

@ceefour
Copy link
Contributor Author

ceefour commented Oct 12, 2016

@leymannx done

@leymannx leymannx mentioned this pull request Oct 12, 2016
@PatrickBauer
Copy link

I understand the idea to ignore the settings.local.php, but not the settings.php file.

In our case, we're using the settings.php file for project-specific settings which should be shared between all developers and the settings.local.php for database-settings and developer specific settings. The server itself has a settings.local.php file too, so no sensitive information gets shared.

@leymannx
Copy link
Collaborator

@PatrickBauer Ah I think now I do understand how it's intended to be used: You completely remove the database-relevant part from settings.php and only have it inside the (ignored) local.settings.php files. Can you confirm that?

@PatrickBauer
Copy link

@leymannx I don't know if its intended to be used that way, but yes, thats how we handle it at our company.

@ceefour
Copy link
Contributor Author

ceefour commented Oct 13, 2016

@PatrickBauer I don't think that's the "official" way of doing it. Since Drupal installer writes to settings.php, not settings.local.php (CMIIW).

Note that each company can still force-checkin settings.php even if ignored, so this PR won't prevent your company from doing what you've been doing. But it makes everyone else safer by default.

@webflo
Copy link
Member

webflo commented Oct 13, 2016

I always commit settings.php without any security related settings (db connection, hash salt) to my repo and enable the include of settings.local.php. Every environment specific settings goes to settings.local.php

@derhasi
Copy link
Member

derhasi commented Oct 14, 2016

Drupal itself has it listed in its .example.gitignore. But I think this really depends on how you deploy and host your project and therefore is very opinionated.

For the short term, adding it to .gitignore might make it more secure for less-experienced users. For the long term, I am currently not sure if and how we should support different approaches within this project. Maybe some sort of scaffolding-tool/installer could assist in the future, but that would have to be maintained too.

@derhasi
Copy link
Member

derhasi commented Oct 14, 2016

Note that each company can still force-checkin settings.php even if ignored

@ceefour, that is no good idea in terms of making your git-repo and process comprehensible. In such a case you really should remove the line from your gitignore.

@ceefour
Copy link
Contributor Author

ceefour commented Oct 14, 2016

@derhasi that's why we have fork in git.
The goal is to give sane defaults. If everyone else wants to custom it (not just gitignore, but anything) they can. Just not the default.

What I mean is everybody has their own version of gitignore anyway, I may want to ignore my IDE's files and whatever. But the files that are part of Drupal stuff, should be handled by default.

@leymannx
Copy link
Collaborator

leymannx commented Oct 14, 2016

Although I totally understand @ceefour as well, I tend to vote for "won't fix" and a line about the suggested procedure (to move db info and hash salt into local.settings) in README so others learn it as well. This seems like the most convenient solution to me.

@Kazanir
Copy link

Kazanir commented Oct 15, 2016

I would also move for won't-fix -- at Platform.sh the methodology we encourage and which everyone seems to use is to commit a settings.php file with project-wide settings and add environment-specific settings to an omitted settings.local.php file. The fact that Drupal's default gitignore file still includes settings.php is more or less an artifact of an earlier age.

@daften
Copy link

daften commented Oct 18, 2016

My 2 cents
The way this project works should not be based on whatever method any cloud provider uses. It should be based on what Drupal ships with and expects. And that is still that settings.php should be excluded. Making new assumptions here will only work confusing. I would propose to actually ignore settings.php by default. Anybody that wants to use this methodology with a settings.local.php can then use it, but will have to adapt the .gitignore file themselves, which shouldn't be a problem.
Basically, agreed with @ceefour

@ostark
Copy link

ostark commented Oct 18, 2016

I agree with @Kazanir - in git/composer based applications ENV var configs are very common.

http://symfony.com/blog/new-in-symfony-3-2-runtime-environment-variables
https://laravel.com/docs/5.3/configuration#environment-configuration

I would love to see this in Drupal as well, especially in drupal-composer.

@gagarine
Copy link
Contributor

gagarine commented Jan 31, 2017

This is a huge security risk especially because drupal installers write directly in settings.php. Please use safe defaults, advance users can always customise to their needs.

I have years of experiences with drupal and git and yet I almost committed the settings.php because in any projects (symfony, drupal, ...) settings are ignored because they can contain highly sensitive information.

This also makes this project look unprofessional :/ and I think it should be removed from the DO documentation until this is fixed.

@wimleers
Copy link

wimleers commented Feb 9, 2017

The way this project works should not be based on whatever method any cloud provider uses. It should be based on what Drupal ships with and expects. And that is still that settings.php should be excluded. Making new assumptions here will only work confusing. I would propose to actually ignore settings.php by default. Anybody that wants to use this methodology with a settings.local.php can then use it, but will have to adapt the .gitignore file themselves, which shouldn't be a problem.

+1

If this wants to deviate from that, then it should be documented super explicitly.

P.S.: When this project is used to deploy Drupal 8 on gandi.net's "Simple Hosting", then this results in your settings.php being overwritten.

@derhasi
Copy link
Member

derhasi commented Feb 14, 2017

@webflo @greg-1-anderson I am pro merging this. Do you have any other opinion on this?

@greg-1-anderson
Copy link
Collaborator

I prefer to commit settings.php as well, and Pantheon expects this. I do wish that Drupal did not write the database credentials to settings.php during the installer by default, but it does. In the balance, setting personal preferences aside, I think that following the example.gitignore file in core is the right thing to do.

@webflo webflo merged commit a271d50 into drupal-composer:8.x Feb 14, 2017
@webflo
Copy link
Member

webflo commented Feb 14, 2017

Thanks you all. I think the installer should write the credentials and hash key to settings.local.php. Maybe we could create a command that moves this stuff around? I will explore some options soonish.

@jcnventura jcnventura mentioned this pull request Mar 8, 2017
dannylamb pushed a commit to islandora-deprecated/drupal-project that referenced this pull request Mar 23, 2017
* Ignore web/sites/*/settings.php and web/sites/*/settings.local.php by default (drupal-composer#178)

* Prefix paths with a slash (drupal-composer#245)

* Update dev dependencies (SA-CORE-2017-001)

https://www.drupal.org/SA-2017-001

* Fix typo in README.md

Fixes drupal-composer#251

* Adding Islandora dependencies (#2)

* Adding Islandora dependencies

Part of Islandora/documentation#474

* Adding the github for claw-jsonld

* Removing requirement on islandora/islandora

islandora/islandora_collection requires it already.

* Removes repository/vcs entries from composer.json and adds requirements (#3)

* Removes repository/vcs entries from composer.json and adds requirements

* Removes islandora and claw-jsonld from composer.json requirements

* Revert to drupal/rdf_ui 1.x-dev version (#4)

* ISSUE-513

Fixes Drupal 8.1.x compatibility issue with drupal/rdf_ui by setting
version fixed to 1.x-dev

* Lock package versions to stuff we know work

* Adding Islandora dependencies (#2)

* Adding Islandora dependencies

Part of Islandora/documentation#474

* Adding the github for claw-jsonld

* Removing requirement on islandora/islandora

islandora/islandora_collection requires it already.

* Removes repository/vcs entries from composer.json and adds requirements (#3)

* Removes repository/vcs entries from composer.json and adds requirements

* Removes islandora and claw-jsonld from composer.json requirements

* Remove and ignore composer.lock; Resolves Islandora/documentation#530

* Pull Request Template (#8)

* address Islandora/documentation#537 (#9)
williambe pushed a commit to williambe/drupal-project that referenced this pull request Apr 28, 2017
shrop pushed a commit to shrop/drupal-project that referenced this pull request Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.