Skip to content

Conversation

@rob006
Copy link
Contributor

@rob006 rob006 commented Jul 9, 2016

Q A
Is bugfix? no
New feature? yes
Breaks BC? no
Tests pass? yes
Fixed issues #69

replaces #60
closes #69

Simplified version of #60 without environment-specific config files.

@samdark
Copy link
Member

samdark commented Jul 25, 2016

@yiisoft/core-developers need opinions about these changes.

composer.json Outdated
},
"scripts": {
"post-install-cmd": [
"php -r \"if(!file_exists('config/console-local.php')){copy('config/templates/console-local.php', 'config/console-local.php');}\"",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer moving these operations to the separate method of yii\composer\Installer class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SilverFire
Copy link
Member

The reasons for creating local configs are not obvious for beginners. The idea should be well documented: why local config is under .gitignore, when should I use local config and when should I the common one

@samdark
Copy link
Member

samdark commented Aug 8, 2016

Yeah. It's not obvious for advanced app users as well despite being documented.

rob006 added 2 commits August 19, 2016 22:45
Conflicts:
	.travis.yml
	tests/codeception/config/acceptance.php
	tests/codeception/config/functional.php
	tests/codeception/config/unit.php
@rob006
Copy link
Contributor Author

rob006 commented Aug 20, 2016

Synced with master and added docs about local configs. Gladly would see some progress for my PRs for yii2-composer to push this forward.

@cebe cebe modified the milestones: 2.0.11, 2.0.10 Oct 20, 2016
@thyseus
Copy link

thyseus commented Nov 29, 2016

+1

@cebe cebe self-assigned this Nov 29, 2016
@cebe
Copy link
Member

cebe commented Nov 30, 2016

I am for adding a simple local config concept to the basic application because it is a needed feature in nearly every project. But it should not make things too complex.
So I'd like to propose the following:

local configs should not use ArrayHelper::merge as it can produce weird situations sometimes when merging sub arrays. I'd prefer a config file included like this:

main.php:

<?php

$config = [
    // ...
];

if (file_exists($localConfig = __DIR__ . '/main.local.php')) {
    require($localConfig);
}
return $config;

main.local.php:

<?php

$config['components']['redis'] = 'yii\redis\Connection';

Array manipulation is more basic and easier to understand than recursive merge.

A minor thing about naming, I prefer main.local.php instead of main-local.php as I find it more intuitive that main belongs to main.local but I have no strong opinon on that one.

@samdark
Copy link
Member

samdark commented Nov 30, 2016

Agree.

@rob006
Copy link
Contributor Author

rob006 commented Nov 30, 2016

local configs should not use ArrayHelper::merge as it can produce weird situations sometimes when merging sub arrays.

About what situations you're talking about?

@cebe
Copy link
Member

cebe commented Nov 30, 2016

example:
main.php:

[
    'params' => [
        'adminUsers' => ['cebe', 'samdark'],
    ],
]

main.local.php:

[
    'params' => [
        'adminUsers' => ['rob006'],
    ],
]

the final result will contain all users. that is not expected.
Basically what we have discussed in yiisoft/yii2#11899 and related.

@rob006
Copy link
Contributor Author

rob006 commented Nov 30, 2016

I would say that this is expected - this is how ArrayHelper::merge() works, and it affects not only configuration. If developer does not understand this, he probably breaks something sooner or later.

My approach also scales much better - if you have many custom local settings, storing them as array is much more readable. If you decide to extract settings shared between console and web app (to main.php config file), you just need to add one line into index.php and yii files. Same for adding environment-specific config files or main-local.php (local config shared between console and web app). With @cebe's approach you need to reinvent wheel again and probably end up with my solution.

About -local.php suffix - this is convention taken from advanced app template. I think it is better to keep this consistent.

@cebe
Copy link
Member

cebe commented Dec 19, 2016

I would say that this is expected - this is how ArrayHelper::merge() works, and it affects not only configuration. If developer does not understand this, he probably breaks something sooner or later.

The main point about not introducing ArrayHelper here is that config is one of the first thing a new user has to deal with to get started, e.g. set up the database. If there are too many files around it is likely to get confused about which files are merged in which way. Configuration should be straight forward in basic app. I agree that ArrayHelper::merge() can be useful in many ways when you have enough experience but it introduces too much complexity for beginners imo.

I feel the need for a third application template between basic and advanced here.

@cebe cebe modified the milestones: 2.0.12, 2.0.11 Dec 19, 2016
@dynasource
Copy link
Member

If there are too many files around it is likely to get confused about which files are merged in which way

the less files the better. IMO its better to start as small as possible and give advice how to expand than the other way around. Alls these config files are not making the life of a developer easier (especially when you don't have experience with it).

In all of my projects I am now trying to minimize the number of config files. The less you have, the less you have to manage.

@cebe
Copy link
Member

cebe commented Dec 20, 2016

How about adding the code that will include a local config file if it exists but do not create the templates?

@dynasource
Copy link
Member

looks good. I like it better than the array merging because its more tightly connected to its owner.

@rob006
Copy link
Contributor Author

rob006 commented Dec 21, 2016

We need at least two templates for local configs: one for DB settings and second for cookieValidationKey in web application config.

@samdark
Copy link
Member

samdark commented Dec 26, 2016

@rob006 rob006 changed the title [WIP] Add support for local config Add support for local config Jan 8, 2017
@rob006
Copy link
Contributor Author

rob006 commented Jan 8, 2017

Sorry guys but implementing changes proposed by @cebe require more work than I suspected, and I don't want to burn more time on this since I'm not using this template anymore. Consider this PR as finished. If you want additional changes on this, someone else has to do it.

@cebe
Copy link
Member

cebe commented Jan 8, 2017

@rob006 no problem, thank you for your contribution! We will decide on how to proceed with this later and make the necessary changes.

@samdark
Copy link
Member

samdark commented Jan 8, 2017

OK. Thank you for contributing!

@rob006 rob006 mentioned this pull request Feb 16, 2017
@samdark samdark added the type:feature New feature label Mar 18, 2017
@samdark samdark modified the milestones: 2.0.13, 2.0.12 Apr 26, 2017
@SilverFire
Copy link
Member

In result of internal discussion, we decided that this PR brings too much complexity as for basic application template so the PR will be closed.

@rob006 thank you for your contribution and sorry that this one is rejected.

@SilverFire SilverFire closed this Aug 9, 2017
@rob006 rob006 deleted the config-local branch August 10, 2017 07:29
@cebe cebe removed this from the 2.0.13 milestone Aug 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cookieValidationKey is not generated after installation

6 participants