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

dotenv v2 #44

Closed
wants to merge 14 commits into from
Closed

dotenv v2 #44

wants to merge 14 commits into from

Conversation

kelvinj
Copy link
Contributor

@kelvinj kelvinj commented Oct 29, 2014

As mentioned in #43 I've tried a few different things:

  1. PSR-4 and PSR-2
  2. namespaced to Dotenv with main public API in Dotenv\Dotenv
  3. Broken functionality into smaller distinct classes with single responsibility
  4. Changed immutability, making it a little less clunky, so now we have load = immutable and overload = mutable. I know you didn't want to copy the ruby dotenv @vlucas but I felt this was pretty elegant.
  5. Parsing of variables now done with a series of filters. To extend parsing as in Allow extensions to override methods and the property #40 the Dotenv\Dotenv class can be extended and a new filter added to the parser. Or indeed a different parser.
  6. I also added a help dotenv() function in order to simplify instantiation, e.g. dotenv()->load(…) as an experiement.
  7. haven't yet implemented a static factory as requested by @bcremer in phpdotenv v2 #43

Comments please.

@kelvinj
Copy link
Contributor Author

kelvinj commented Oct 29, 2014

Oh, and I haven't updated the readme at all.

@vlucas
Copy link
Owner

vlucas commented Oct 30, 2014

Man, solid start. Huge kudos. I am wrapping up some work right now, and won't be able to thoroughly look this over until the first week of November (a few days). I'll be able to give you some more in-depth feedback then.

The one thing I planned on doing in v2 that I don't see here is that I was strongly considering adding different parser types, so that Dotenv could also support a simpler .env.php that just returns an array if people wanted to use that instead. Thoughts on that?

@kelvinj
Copy link
Contributor Author

kelvinj commented Oct 30, 2014

@vlucas yep, good idea. ok. In a way, that's not really a parser… so maybe I'm missing a concept.

I'll have a think.

@GrahamCampbell
Copy link
Collaborator

@GrahamCampbell
Copy link
Collaborator

Also, big update here: https://github.com/brightmachine/phpdotenv/pull/3.

@kelvinj
Copy link
Contributor Author

kelvinj commented Oct 30, 2014

@GrahamCampbell sorry, just did a large refactor before seeing your PRs. 😞

@GrahamCampbell
Copy link
Collaborator

I'll rebase the first 2 right now...

@kelvinj
Copy link
Contributor Author

kelvinj commented Oct 30, 2014

A few more updates:

  • concepts a little more clear, Dotenv, Variable, and classes which load variables.
  • supports multiple loaders
  • a new .env.php loader for loading php environment variables from arrays
  • initial tests for php loader

One change I think I'll make is to be smarter about what happens when you call ->load('dir/path', null);. Currently defaults to .env but we could loop over each loader looking for file, instead of assuming.

@kelvinj
Copy link
Contributor Author

kelvinj commented Oct 30, 2014

@GrahamCampbell cheers.

@GrahamCampbell
Copy link
Collaborator

Rebased.

@kelvinj
Copy link
Contributor Author

kelvinj commented Nov 6, 2014

Didn't clarify that now Dotenv will now look for default files based on the extensions supported by the loaders, e.g. when ->load('dir/path', null); called.

Question for @vlucas re: the .env.php loader – seeing as putenv() takes only a string, and .env is dealing in strings, how do you want .env.php to handle variable values?

  1. Cast all values as strings?
  2. Assert that all values are strings?
  3. Allow other values and attempt to cast to string when using putenv()
  4. Do nothing? What will be will be.
  5. Some other option…

@vlucas
Copy link
Owner

vlucas commented Nov 6, 2014

I would say do (1) because that's now they are going to be when retrieved back with getenv.

@GrahamCampbell
Copy link
Collaborator

Any news on this guys?

- boolean values `true && false` handled as strings `'true' && 'false'`
- other scalars cast to strings
- other complex types return empty string
@kelvinj
Copy link
Contributor Author

kelvinj commented Nov 27, 2014

Just pushed up last change around casting .env.php values as strings.

After this, just need some more feedback from @vlucas

If he's happy with this, then it's a matter of updating the docs and getting v2 out.

@GrahamCampbell
Copy link
Collaborator

It also would be cool to have a final v1.0.10 release before this is merged.

@vlucas
Copy link
Owner

vlucas commented Nov 28, 2014

I am doing a complete review of all this code now.

@GrahamCampbell
Copy link
Collaborator

Great. :)

@vlucas
Copy link
Owner

vlucas commented Dec 2, 2014

Okay - I have reviewed it all, and only had a few minor changes. Next, I am going to use it in some projects of mine and see how that goes before releasing. Thanks for all your hard work, @kelvinj!

@prisis
Copy link

prisis commented Dec 2, 2014

👍

@kelvinj
Copy link
Contributor Author

kelvinj commented Dec 2, 2014

Cool.

Any chance of merging this to a v2 branch?

@vlucas
Copy link
Owner

vlucas commented Dec 2, 2014

@kelvinj
Copy link
Contributor Author

kelvinj commented Dec 2, 2014

@vlucas thanks.

@GrahamCampbell
Copy link
Collaborator

This can be closed then?

@vlucas
Copy link
Owner

vlucas commented Dec 2, 2014

I do want to talk for a minute about the required function before I release an official v2. Issue #26 proposes that the required function should also check for empty values. I made the change in v1, and then ended up rolling it back because it added unexpected behavior that was breaking for some users. So the question is - should we add that capability back in, and how if so, how? A separate method? An additional boolean parameter?

@kelvinj
Copy link
Contributor Author

kelvinj commented Dec 2, 2014

I do find the current required method a bit clunky.

What about introducing another concept, variable assertion?

<?php
$dotenv->assert('VAR')->exists();
$dotenv->assert('VAR')->notEmpty();
$dotenv->assert('VAR')->contains(array('option1', 'option2'));

@vlucas
Copy link
Owner

vlucas commented Dec 3, 2014

Hmmm... I kind of like that. It would have to also support multiple variables though, so it would not be overly verbose. I would also like to allow chaining, like so:

$dotenv->assert(['FOO', 'BAR', 'BAZ'])->exists()->notEmpty();

@kelvinj kelvinj mentioned this pull request Dec 3, 2014
@kelvinj
Copy link
Contributor Author

kelvinj commented Dec 3, 2014

@vlucas checkout #50 - instead of exists() went with notNull() … I think that's clearer.

@GrahamCampbell
Copy link
Collaborator

This can be closed then?

Surely there should be another pull request created to merge the v2 from this repo to the master of this repo?

@GrahamCampbell
Copy link
Collaborator

Branch proposal:

Create a 1.1 branch, and a master and alias the master to 2.0-dev. The 1.1 branch doesn't need aliasing as packagist will make it 1.1-dev automatically.

@vlucas
Copy link
Owner

vlucas commented Dec 11, 2014

I just merged @kelvinj's code and pushed a v2 branch. I am in the process of testing it now and will publish it as v2 shortly.

@vlucas
Copy link
Owner

vlucas commented Dec 11, 2014

Feel free to test out the new version by using version 2.x-dev:
https://packagist.org/packages/vlucas/phpdotenv

@vlucas
Copy link
Owner

vlucas commented Dec 11, 2014

Closing this PR as I have merged the code manually into branch v2 instead of through this UI.

@vlucas vlucas closed this Dec 11, 2014
@kelvinj
Copy link
Contributor Author

kelvinj commented Dec 11, 2014

@vlucas let me know what you need to get v2 out the door. Readme? Changelog?

@vlucas
Copy link
Owner

vlucas commented Dec 11, 2014

I do need a README update, but I plan on doing that in a few hours. I couldn't possibly ask you to do any more for v2! :)

@kelvinj
Copy link
Contributor Author

kelvinj commented Dec 11, 2014

Probably a good thing too from your perspective… when it comes to explaining things in plain English, well, let's say it's something I'm continually trying to improve upon.

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.

None yet

4 participants