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

BaseConfig should support array values with dot syntax #454

Closed
lonnieezell opened this issue Mar 24, 2017 · 21 comments
Closed

BaseConfig should support array values with dot syntax #454

lonnieezell opened this issue Mar 24, 2017 · 21 comments
Labels
dev enhancement PRs that improve existing functionalities

Comments

@lonnieezell
Copy link
Member

Currently, I don't believe it loads those correctly. Check loading database group settings for a specific example.

@lonnieezell lonnieezell added the bug Verified issues on the current code behavior or pull requests that will fix them label Mar 24, 2017
@lonnieezell lonnieezell added this to the Pre-Alpha 2 milestone Mar 24, 2017
@SmolSoftBoi
Copy link
Contributor

I ran into the same issue when created a basic mail library for my application, seems to be because loading using dot syntax isn't recursive and only looks a few levels deep?

@lonnieezell
Copy link
Member Author

That's what I thought it was, but I just realized that it seems to work fine for database config options, which is very similar to the mail options I've been working on. So this will probably take a little more investigation. :)

@ghost
Copy link

ghost commented Mar 29, 2017

Hello, just a note on this same topic.

I am using current latest code (#df60c1b2) and I cannot load database config from .env file.
I've only tested it on migrations.

.env:

database.default.hostname = 'localhost'
database.default.database = 'abc'
database.default.username = ''
database.default.password = ''
database.default.DBDriver = 'Postgre'

Database.php:

	public $default = [
		...
		'hostname'     => 'localhost',
		'username'     => '',
		'password'     => '',
		'database'     => 'def',
		'DBDriver'     => 'MySQLi',
		...
	];

Migration tries to connect using mysql, it doesn't use the postgres credentials I specified in .env file.

Also, even if i provide correct credentials in Database.php, migration still fails:

An uncaught Exception was encountered

Type:        Error
Message:     Call to undefined function CodeIgniter\Database\Postgre\pg_connect()
Filename:    [codeigniter4]/system/Database/Postgre/Connection.php
Line Number: 97

OS: macOS 10.12.4
PHP: 7.1.3

And, yes, I have postgres, mysql php extensions installed. (My other PHP projects successfully connects to Postgres database)

@lonnieezell
Copy link
Member Author

@DavisMiculis Where is your .env file? It sounds like it's not loading it. It should be under the root directory, not application directory where it used to live. I just stepped through a couple of examples on the database and it's reading settings correctly.

Additionally, it seems to be handling array values with dot syntax just fine, though I need to do some more tests to see why it wasn't working with my mail stuff.

@lonnieezell
Copy link
Member Author

Ok, looks like my mail stuff wasn't working simply because it was deeper than the database config file is. So, the solution is either to simplify the mail config file or make BaseConfig recursive to deal with any depth - and that's probably the best solution.

@ghost
Copy link

ghost commented Mar 30, 2017

Didn't even notice it was moved. Ok, so I moved .env file to root directory.
.env file still contains config for Postgres, while Config/Database.php has the default Mysql

Now here's something interesting:
If I cd into /public and run php index.php migrate it uses Postgres (Controller for migrations)
If I cd into root and run php ci.php migrate:refresh it uses Mysql.

But it still throws me errors:

davis@MacBook-Pro:~[codeigniter4]/public$ php index.php migrate

An uncaught Exception was encountered

Type:        Error
Message:     Call to undefined function CodeIgniter\Database\Postgre\pg_connect()
Filename:    [codeigniter4]/system/Database/Postgre/Connection.php
Line Number: 97
davis@MacBook-Pro:~[codeigniter4]$ php ci.php migrate:refresh

CodeIgniter CLI Tool - Version 4.0-dev - Server-Time: 2017-03-31 01:25:23am


An uncaught Exception was encountered

Type:        ErrorException
Message:     mysqli::real_connect(): (HY000/2002): No such file or directory
Filename:    [codeigniter4]/system/Database/MySQLi/Connection.php
Line Number: 183

@jim-parry
Copy link
Contributor

PR 618 has unit tests for Config, including dot array syntax.
Does it resolve this issue?

@lonnieezell
Copy link
Member Author

@jim-parry IIRC, the issue is that it needs to handle it recursively. I believe it does it one or two levels deep only currently.

The other option is that we ditch the automatic setting of values since it is fairly magic, and provide a custom env() function to read values from the three places dotenv sets them. I've been thinking about that lately, but not sure what everyone else thinks....

@ujjaldutta
Copy link

can you allow to put function in database.php to use like 'database' => env('database.default.username', 'testdb'),
Its now not allowing me to use env file and function

@lonnieezell
Copy link
Member Author

@ujjaldutta I use the env file for database config all of the time. Here's an example from my .env file:

### MYSQL LOCAL
database.default.hostname = localhost
database.default.database = ci4
database.default.username = root
database.default.password = root
database.default.DBDriver = MySQLi
database.default.DBPrefix = db_

And the db config is all default.

However, there is an env method in the Common.php file already if you want to use that instead.

@lonnieezell
Copy link
Member Author

@jim-parry What are your thoughts on removing the magic in the BaseConfig file altogether and just using the env() method?

@ujjaldutta
Copy link

Hi,
Can anyone tell me why in doc the function is $routes->resources . I have checked the function is $routes->resource not resources().
Please tell me if I am doing any mistake.
Thanks.

@lonnieezell
Copy link
Member Author

@ujjaldutta Probably a typo.... and one that doesn't have anything to do with this issue. :)

@lonnieezell
Copy link
Member Author

@jim-parry Since I haven't heard back an opinion just yet, I'm going to assume that means you're cool with scrapping the magical nature of BaseConfig. :)

Seriously, though, I'm leaning very heavily toward doing that because it's a magic box that is hard to debug when it doesn't work quite right. using env('DB_USER') in the config files is:

  • a) more explicit
  • b) simpler to understand when you take the project over

If you don't have any issues, or if I don't hear from you in the next few days, I'll go ahead and remove that feature.

@jim-parry
Copy link
Contributor

@lonnieezell Sorry - I thought I had commented, but I agree - it isn't here :-/
I have no problem simplifying the config, or things that rely on "magic" :)

@lonnieezell
Copy link
Member Author

No worries. The main problem I have isn't that it's magic, it's that it's unclear how it's happening, and a PITA to debug when you're trying to get the right config value in the .env file.

Since you're good either way, I'll go ahead and simplify that aspect. Won't lose any power and is clearer to boot.

@lonnieezell
Copy link
Member Author

Or maybe not. I remember why I did that now. You can't implement expressions as default values. This is the one time that using a class for a config value really gets in the way, as compared to a simple array. :(

Guess it will stay. The alternative is ugly.

@lonnieezell
Copy link
Member Author

@ujjaldutta For your previous comment: no, we cannot allow use of env() to be assigned as the value because PHP won't allow you to use an expression as a default value.

Instead, you can do it inside the constructor, though.

public function __construct()
{
    parent::__construct();
    $this->database = env('database.default.username', 'testdb');
}

@jim-parry
Copy link
Contributor

@lonnieezell Can we close this? anything else to fix or is this still a release blocker?

@lonnieezell
Copy link
Member Author

I wouldn't say it's a release blocker, but is still a feature that would be nice to have, since dot array syntax works elsewhere. The main, thing, though, is that it should be updated to be recursive so it can handle more than 1 or 2 levels deep that it currently does.

@jim-parry jim-parry modified the milestones: Pre-Alpha 2, 4.0.0-alpha Sep 21, 2018
@jim-parry jim-parry modified the milestones: 4.0.0-alpha, 4.0.1? Nov 13, 2018
@jim-parry jim-parry added enhancement PRs that improve existing functionalities and removed bug Verified issues on the current code behavior or pull requests that will fix them labels Nov 16, 2018
@jim-parry jim-parry removed this from the 4.0.1? milestone Nov 16, 2018
@jim-parry jim-parry added the dev label Mar 6, 2019
@lonnieezell
Copy link
Member Author

Fixed with #2082

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev enhancement PRs that improve existing functionalities
Projects
None yet
Development

No branches or pull requests

4 participants