Skip to content

[DependencyInjection] Add urlencode function to EnvVarProcessor #52369

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
Merged
merged 1 commit into from
Nov 18, 2023
Merged

[DependencyInjection] Add urlencode function to EnvVarProcessor #52369

merged 1 commit into from
Nov 18, 2023

Conversation

crtl
Copy link
Contributor

@crtl crtl commented Oct 30, 2023 •
edited by OskarStark
Loading

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
License MIT

This PR adds urlencode to the default EnvVarProcessor to allow encoding environment variables in config files.
The comes handy when environment variables are provided by the OS and not through .env because those values are often not compatible with DSN syntax for doctrine or mailer components.

In my case our app was deployed on an AWS stack and environment variables for SMPT and database where automatically provided in the deployment process and replaced in the respective .env files.
Because those values where not encoded they would cause issues with DSN format requireing me to commit them in my .env.prod which circumvents the purpose of the whole system.

Usage:

# config/packages/mailer.yaml
framework:
    mailer:
        dsn: 'smtp://%env(urlencode:SMTP_USER)%:%env(urlencode:SMTP_PASSWORD)%@%env(urlencode:SMTP_HOST)%?encryption=%env(SMTP_ENCRYPTION)%&port=%env(SMTP_PORT)%&auth_mode=login'

@carsonbot carsonbot added this to the 6.4 milestone Oct 30, 2023
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

GromNaN
GromNaN reviewed Oct 30, 2023
View reviewed changes
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

Great idea. I've already had a project that crashed because a & slipped into an automatically generated password.

GromNaN
GromNaN approved these changes Oct 31, 2023
View reviewed changes
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

SGTM.
We are feature freeze in preparation of 6.4 and 7.0. This will be for version 7.1.

@OskarStark OskarStark changed the title [DependencyInjection] Adds urlencode function to EnvVarProcessor [DependencyInjection] Add urlencode function to EnvVarProcessor Oct 31, 2023
@OskarStark OskarStark modified the milestones: 6.4, 7.1 Oct 31, 2023
@fabpot fabpot force-pushed the feature/urlencode-env-in-config branch from aab25b5 to 1439858 Compare November 18, 2023 18:29
@fabpot
Copy link
Member

fabpot commented Nov 18, 2023

Thank you @crtl.

@fabpot fabpot merged commit 34edf43 into symfony:7.1 Nov 18, 2023
@fabpot fabpot mentioned this pull request Nov 18, 2023
Closed
@fabpot fabpot mentioned this pull request May 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@GromNaN GromNaN GromNaN approved these changes

@OskarStark OskarStark OskarStark approved these changes

Assignees
No one assigned
Labels
DependencyInjection Feature Status: Reviewed
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
5 participants
@crtl @carsonbot @fabpot @GromNaN @OskarStark