8000 [Dotenv][WebServerBundle] Possible BC break when overriding previously loaded variables · Issue #24078 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Dotenv][WebServerBundle] Possible BC break when overriding previously loaded variables #24078

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.

Sign up for GitHub

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

Jump to bottom
Closed
javiereguiluz opened this issue Sep 3, 2017 · 4 comments
Closed

[Dotenv][WebServerBundle] Possible BC break when overriding previously loaded variables #24078

javiereguiluz opened this issue Sep 3, 2017 · 4 comments
Labels
BC Break Dotenv WebServerBundle

Comments

@javiereguiluz
Copy link
Member
Q A
Bug report? yes
Feature request? no
BC Break report? yes
RFC? no
Symfony version 3.3

As reported by @tiger-seo in #23799:

  1. i am grateful for all your hard work, but this change is a breaking change and it is not mentioned :(

  2. to give you more info why it is a BC, please see our env_autoload.php, which is used instead of autoload.php:

<?php

/** @var \Composer\Autoload\ClassLoader $loader */

use Symfony\Component\Dotenv\Dotenv;
use Symfony\Component\Dotenv\Exception\PathException;

$loader = require __DIR__ . '/autoload.php';

$dotenv = new Dotenv;

try {
    // existing environment variables are never overridden
    $dotenv->load(__DIR__ . '/config/.env');
} catch (PathException $e) {
}

// existing environment variables are never overridden
$dotenv->load(__DIR__ . '/config/.env.dist');

return $loader;
  1. @tiger-seo, I think you could try to load the .env.dist earlier than the .env:
// existing environment variables are never overridden
$dotenv->load(__DIR__ . '/config/.env.dist');

try {
    // existing environment variables are never overridden
    $dotenv->load(__DIR__ . '/config/.env');
} catch (PathException $e) {
}

return $loader;
  1. yes, this is exactly how we've fixed it
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Sep 3, 2017 •
edited
Loading

I'm not sure we should do anything here. Fixing this is a BC break. Since the component is new, it's easier for ppl to update their code. I know this is not the policy and if we've noticed this change earlier, we would have thought twice about it.

Yet, looking at the broken code, it is already very strange to me to rely on an exception to "fallback". I'd expect ppl to do a file_exists check instead.

@voronkovich
Copy link
Contributor

@nicolas-grekas, we could fix it by adding a BC break note.

@nicolas-grekas
Copy link
Member

Would you mind opening a PR doing so?

@voronkovich voronkovich mentioned this issue Sep 3, 2017
Closed
@voronkovich
Copy link
Contributor

@nicolas-grekas, I've created a PR: #24081

nicolas-grekas added a commit that referenced this issue Sep 5, 2017
@nicolas-grekas
minor #24081 [Dotenv] Add a BC break note (voronkovich)
703a84c 
This PR was squashed before being merged into the 3.3 branch (closes #24081).

Discussion
----------

[Dotenv] Add a BC break note

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  |no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24078
| License       | MIT

Commits
-------

66621cc [Dotenv] Add a BC break note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BC Break Dotenv WebServerBundle
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@javiereguiluz @nicolas-grekas @voronkovich
0