josegonzalez/php-dotenv

Proposition from m1/env

m1 opened this issue · 21 comments

m1 commented

Hey there,

Just saw your comment on reddit, I'm the author of m1/env

Well basically my proposition is basically what I say in my comment

My aim of building m1/env was that other .env libraries would then use my library for the parsing and their library could be used for the loading into superglobals and other stuff like that. Just wanted to do one thing and do it well. My inspiration behind this project was symfony/yaml

This will also mean less work for you to bring up to speed with my package, so therefore more time for you to spend on other features, also you won't have to worry about the parser as this will be my job.

If you would like to do this, would love to work with you. No worries if not.

Thanks,
Miles

I'm not opposed. Do you have any benchmarks regarding parsing speed for the packages? Just curious, won't really impact my decision.

m1 commented

Yeah I've done a few benchmarks with ab.

For the benchmarks I pretty much did:

I set up very basic php files for each:

m1/env:

<?php 

require __DIR__ . '/vendor/autoload.php';
$env = new M1\Env\Env('basic.env');

josegonzalez/php-dotenv:

<?php
require __DIR__ . '/vendor/autoload.php';

$env = (new josegonzalez\Dotenv\Loader('basic.env'))
              ->parse()
              ->toArray();

basic.env:

# Comments are done like this

# Standard key=value
TEST1 = value
TEST2 = value
TEST3 = value # You can also comment inline like this

# Strings
TEST4 = "value"

## The value of the below variable will be TK4 = "value value"
TEST5 = "value value" "this sentence in quotes will not be counted"

## Escape newline
TEST6 = "value \n value"

## Escape double quotes
TEST7 = "value \"value\" value"

## You can also exchange any of the above for single quotes, eg:
TEST8 = 'value'
TEST9 = 'value \' value \' value'

# Numbers
TEST10 = 1
TEST11 = 1.1
TEST12 = 33 33 # Will output as a `string` -- not a number as two numbers are given
TEST13 = "33" # 33 -- `string` type

# Bools -- All of the below are valid booleans
TEST14 = true
TEST15 = false
TEST16 = yes
TEST17 = no

## Booleans are case-insensitive
TEST18 = True
TEST19 = False
TEST20 = YES
TEST21 = NO

TEST22 = "true" # "true" -- `string` type
TEST23 = "YES" # "YES" -- `string` type
TEST24 = 'NO' # "NO" -- `string` type

# Null values
TEST25 =
TEST26 = null

# Variables
TEST27 = 'hello'
TEST28 = ${TEST27} # 'hello'

TEST29 = 1
TEST30 = 2
TEST31 = ${TEST29}   # 1 -- `int` type
TEST32 = "${TEST29}" # 1 -- `string` type
TEST33 = ${TEST29} ${TEST30} # 1 2 -- `string` type

TEST34 = foo
TEST35 = bar
TEST36 = ${TEST34}/${TEST35} # foo/bar -- `string` type

TEST37 = "foo"
TEST38 = 'bar'
TEST39 = "hello ${TEST37} and ${TEST38}" # hello foo and bar -- `string` type

TEST40 = true
TEST41 = false
TEST42 = ${TEST40} # true -- `bool` type
TEST43 = ${TEST40} ${TEST41} # true false -- `string` type

TEST44 = null
TEST45 = ${TEST44} # null -- `null` type
TEST46 = "${TEST44}" # '' -- `string` type

TEST47=foo
TEST48=${TEST47:=bar}
TEST49=${TEST50:=foo}
TEST51=${TEST52:-foo}
TEST53=null
TEST54=${TEST53=foo}
TEST55=null
TEST56=${TEST55-foo}
TEST57=${TEST58:=""}
TEST59=${TEST60:=null}
TEST61=${TEST62:=true}

# Comments
TEST63 = hello # comment
TEST64 = "hello # comment"
TEST65 = "hello" #comment
TEST66 = #comment
TEST67 = "#comment"

The figures for ab -t 10 -c 10 were:

m1/env:

Benchmarking env1.dev (be patient)
Completed 5000 requests
Completed 10000 requests
Finished 12057 requests


Server Software:        nginx/1.4.6
Server Hostname:        env1.dev
Server Port:            80

Document Path:          /
Document Length:        0 bytes

Concurrency Level:      10
Time taken for tests:   10.001 seconds
Complete requests:      12057
Failed requests:        0
Total transferred:      2507856 bytes
HTML transferred:       0 bytes
Requests per second:    1205.60 [#/sec] (mean)
Time per request:       8.295 [ms] (mean)
Time per request:       0.829 [ms] (mean, across all concurrent requests)
Transfer rate:          244.89 [Kbytes/sec] received

josegonzalez/php-dotenv:

Benchmarking env4.dev (be patient)
Completed 5000 requests
Completed 10000 requests
Finished 13339 requests


Server Software:        nginx/1.4.6
Server Hostname:        env4.dev
Server Port:            80

Document Path:          /
Document Length:        0 bytes

Concurrency Level:      10
Time taken for tests:   10.000 seconds
Complete requests:      13339
Failed requests:        0
Total transferred:      2774512 bytes
HTML transferred:       0 bytes
Requests per second:    1333.86 [#/sec] (mean)
Time per request:       7.497 [ms] (mean)
Time per request:       0.750 [ms] (mean, across all concurrent requests)
Transfer rate:          270.94 [Kbytes/sec] received

Yours does beat mine in the number of requests, but that's probably down to parameter expansions and a few other things.

If we did do this, I would give you collaborator access to env if you wanted so you could push upstream to the parser.

Woot I'm regex crushing you.

We should probably just make a new org. Call it dotenv.

We can move your project to be php-env-parser and mine to be php-dotenv. Obviously I would gut all of my parsing and use your project under the hood.

Why the prefix? So we can ask all the other language maintainers to move as well, so that people can more easily discover this stuff. Idk, maybe a lofty goal, but shrug

babby steps, amirite?

m1 commented

Baby steps indeed. Right, for the time being do you want to gut your parsing stuff and make my parser a dependency?

Then we can get onto making a org then ruling the world?

Can you provide an interface wherein your version is given the contents of the file instead of the path to the file? I'd like to continue handling that bit on my own if at all possible, and it seems like I can't if I use your parser.

You can of course keep that sort of stuff in your project, but atm even the Parser class has Reader functionality in it :(

m1 commented

Yes that's fine, that's actually how symfony/yaml works if I'm understanding you correctly.

Can you be a bit more specific?

I guess I would remove the file_get_contents from the Parser class, and let it accept an already read string.

One thing I noticed is that your Env class does some ini_set magic as a wrapper around the parser, so I'm not completely certain that your parser is standalone, since it seems to require side-effects from an external class. shrug.

Note: one thing my parser supports is prefixed export statements. That is, the .env file can be sourced in on bash, which is useful for background classes. Seems like that isn't supported for yours. Should I file a bug?

m1 commented

The ini_set is fine, it's just for detecting of line endings. I was talking to someone the other day about this and found out I can pretty much remove it without any side effects.

Yeah I was meaning to bring export to mine, I can get this change round pretty quick.

Alright let me know. I think my tests should mostly pass - other than the {} issue - once thats true, so I won't need to do any more work.

For now I'll use your Env interface, but if I could drop that and use Parser directly, that would be a slight win in terms of removing indirection.

Fwiw, this is what I'm using for export statements. It's taken more or less directly from dokku's config parsing.

m1 commented

Ohh thanks for the regex, will use that.

I think I'll go with the using the Parser directly, what would you need/want from that in terms of what functionality do you want from it what isn't already there?

Here is my parser. I don't throw exceptions - your parser does, maybe mine is just more lenient? - and I automatically change unquoted strings to their php counterparts. That's more or less it.

Basically what I would do is:

  1. install my package
  2. checkout the drop-parser branch
  3. composer install
  4. modify the composer-version of m1/env to add features
  5. run my project's tests
  6. go back to step 4 until tests pass

Once that is through, I would say your version is a drop-in replacement for my Parser, except with more features.

m1 commented

Right. I've added in export functionality and remove the ini_set stuff

m1 commented

Will try your tests against mine, what's the need in changing the composer version though?

Also I should change my parser to take the content rather than the file right?

m1 commented

Do you need a function to get back the content, or will just the parse function in parser suffice?

I currently peg to dev-master for your dependency, so once it goes to master, I'll get it in my branch. We'll want to have a stable release of your library at some point, and then I can do 1.* as the dependency.

I think thats the way to do it. Since your library is just a parser, it shouldn't matter where the contents are from. Hell, we could make a small webservice for this :D

Just an array of key => value info will be fine.

m1 commented

Okay excellent! I'll start testing your package with my parser and probably get it working by tomorrow. 😁

m1 commented

So I got your library working with my parser. Shall I open a pull request?

Are you on irc anywhere? I'm savant on freenode (you can hit me up in #cakephp).