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

Add hint about variables_order ini setting required #17267

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

DracoBlue
Copy link
Contributor

@DracoBlue DracoBlue commented Sep 13, 2022

I read about real environment variables always winning, when using dotenv - but my setup did not work like this. And I noticed that my variables_order ini setting was set to the default of debian production settings, which does not expose $_ENV by default.

Thus I added a sentence right behind stating that this depends on $_ENV superglobal being filled, which also needs variables_order to contain an uppercase E.

``.env`` files.
``.env`` files. This behavior depends on
`variables_order <http://php.net/manual/en/ini.core.php#ini.variables-order>`_ to
contain an ``E`` to expose the ``$_ENV`` superglobal.
Copy link
Contributor

Choose a reason for hiding this comment

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

Friendly ping @stof @nicolas-grekas

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's wait for their review because this change is really important, so we must be sure it's correct. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

The Dotenv has a fallback reading from $_SERVER (for env variables not starting with HTTP_ to avoid security issues due to confusion with HTTP headers), but I'm not sure the CLI populates it. So it should support the case of not having E in the order as long as you don't rely on env variables starting with HTTP_

Copy link
Member

Choose a reason for hiding this comment

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

@stof we've just received another user report from someone facing an issue because of this. So, I think we should merge this PR, unless you are against it. Thanks.

@phansys
Copy link
Contributor

phansys commented Jun 14, 2023

I think this change is fine. I recently faced an issue regarding the absence of the "E" on the variables_order setting and I spent quite a bit of time debugging to reach the same conclusion than I now found in this PR.

@OskarStark OskarStark added this to the 6.2 milestone Jun 16, 2023
@OskarStark OskarStark changed the base branch from 6.1 to 6.2 June 16, 2023 06:11
@OskarStark
Copy link
Contributor

OskarStark commented Jun 16, 2023

Thanks @DracoBlue.

Note: I merged this in 6.2 instead

@OskarStark OskarStark merged commit 067a41a into symfony:6.2 Jun 16, 2023
@DracoBlue DracoBlue deleted the patch-2 branch June 16, 2023 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants