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

[DI] Add an url EnvProcessor #28975

Merged
merged 1 commit into from Mar 18, 2019

Conversation

Projects
None yet
9 participants
@jderusse
Copy link
Contributor

commented Oct 25, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR symfony/symfony-docs#11128

This PR add a new env processor url to convert an URL (or DSN) into an array.

The main goal is to simplify the project configuration and reduce the number of env variable when working with bundle which are not able to deal with DSN
(pick some random project in symfony/recipes-contrib: https://github.com/symfony/recipes-contrib/blob/master/facile-it/mongodb-bundle/0.6/manifest.json or https://github.com/symfony/recipes-contrib/blob/master/wouterj/eloquent-bundle/1.0/manifest.json)

# before
MONGODB_HOST=localhost
MONGODB_PORT=27017
MONGODB_USER=
MONGODB_PASSWORD=
MONGODB_DB=symfony

mongo_db_bundle:
    data_collection: '%kernel.debug%'
    clients:
        default:
            hosts:
            - { host: '%env(MONGODB_HOST)%', port: '%env(int:MONGODB_PORT)%' }
            username: '%env(MONGODB_USER)%'
            password: '%env(MONGODB_PASSWORD)%'
            connectTimeoutMS: 3000
    connections:
        default:
            database_name: '%env(MONGODB_DB)%'


# after
MONGODB_DSN=mongodb://localhost:27017/symfony

mongo_db_bundle:
    data_collection: '%kernel.debug%'
    clients:
        default:
            hosts:
            - { host: '%env(key:host:url:MONGODB_DSN)%', port: '%env(key:port:url:MONGODB_DSN)%' }
            username: '%env(key:user:url:MONGODB_DSN)%'
            password: '%env(key:pass:url:MONGODB_DSN)%'
            connectTimeoutMS: 3000
    connections:
        default:
            database_name: '%env(key:path:url:MONGODB_DSN)%'

Added also a query_string processor to parse query string

DATABASE_DSN=mysql://localhost/db?charset=utf8

foo:
  bar:
    charset: '%env(key:charset:query_string:key:query:url:DATABASE_DSN)%'

@jderusse jderusse force-pushed the jderusse:env-url branch from 2ae1698 to 345b19b Oct 25, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

Before reading the code, I was expecting this to be a processor to do a parse_str().
Now that I read it I see it's for parse_url().
I think we should ship both in this PR because they are complementary - that will force us to find the least confusing names for each :)

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 25, 2018

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

@jderusse even I appreciate your contribution, I'm going to share an (unpopular?) opinion about this "env processor" trend. I think it's a mistake and makes the YAML config files even harder to understand. I know we need to support something like this ... but I prefer if we focused instead on switching to PHP config by default.

Before

mongo_db_bundle:
    clients:
        default:
            hosts:
                - { host: '%env(key:host:url:MONGODB_DSN)%', port: '%env(key:port:url:MONGODB_DSN)%' }
            username: '%env(key:user:url:MONGODB_DSN)%'
            password: '%env(key:pass:url:MONGODB_DSN)%'
    connections:
        default:
            database_name: '%env(key:path:url:MONGODB_DSN)%'

After

$mongo = parse_url(env('MONGODB_DSN'));

return ['mongo_db_bundle' => [
    'clients' => [
        'default' => [
            'hosts' => [
                'host' => $mongo['host'], 'port' => $mongo['port'],
            ],
            'username' => $mongo['user'] ?? null,
            'password' => $mongo['pass'] ?? null,
        ]
    ],
    'connections' => [
        'default' => [
            'database_name' => $mongo['path'],
        ]
    ]
];
@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

parse_url(env('MONGODB_DSN'));

fundamentally the goal of %env()% is to not fetch envs during compile time.

But for PHP config a env builder might be nice:

'database_name' => env('key:path', 'url', 'MONGODB_DSN'),

to at least make the strings more readable.

@andersonamuller

This comment has been minimized.

Copy link

commented Oct 25, 2018

@javiereguiluz That is a very popular opinion to me :)

@ro0NL Isn't it possible to have something like the code below?

// services.php
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;

return function (ContainerConfigurator $configurator) {
    $parameters = $configurator->parameters();

    $defaultMongoDbUrl = 'mongodb://localhost:27017/symfony?ssl=false';

    $parameters->set(
        'database_name', 
        env('MONGODB_DSN', $defaultMongoDbUrl)->url()->path()
    );
    $parameters->set(
        'database_secure', 
        env('MONGODB_DSN', $defaultMongoDbUrl)->url()->query('ssl', true /* default */)
    );

    // other examples
    $parameters->set(
        'security_secret', 
        /* without default value, it will throw exception if does not exist at runtime */
        env('APP_SECRET_FILE')->file()->trim() 
    ); 
    $parameters->set('locale', env('APP_LOCALE', 'pt_BR'));
    /* defaut values are always a string */
    $parameters->set('account_threshold', env('APP_THRESHOLD', '10')->int());
};

also for the other PR #28976

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

@andersonamuller sure, whatever API that can build the string for us :)

the question is; how much code do we want to add/maintain only to generate a string.

@jderusse

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

@ro0NL I once opened a RFC for this #28084

@javiereguiluz of course we can do everything with php config file. Does Symfony want to promote use of yaml config file for simple case and php for complex one. This is not my call ;-) But in that case, what is the purpose of envProcessor like 'key', 'json', 'file'?

@jderusse

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

@nicolas-grekas done. Took url and query.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

Just to clarify: I think that YAML is fantastic for config files. The problem is when you try to add PHP-like features to YAML. If we continue doing that, it's better to switch to PHP.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

@javiereguiluz please don't hijack the topic, @ro0NL is right here: your proposal misses a lot of context to have it work - maybe we can make it work, but that's a huge task, that's what I mean.

@jderusse jderusse force-pushed the jderusse:env-url branch from 91afd40 to cdf9e79 Nov 5, 2018

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

@jderusse your before/after example is perfect for Symfony Docs. Just asking about the missing parts:

  • Would this feature require to enable/configure anything when using it in a Symfony full-stack app? (I guess it won't)
  • Would this require some changes when using it as a stand-alone component? I guess it does ... please show some example of how to activate this. Thanks!
@jderusse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

@javiereguiluz This feature uses exactly the same "workflow" than key, json, int, bool, or file processor. It don't use new Class or thing to instantiate.

You can use it without the framework like this:

use Symfony\Component\DependencyInjection\ContainerBuilder;

$containerBuilder = new ContainerBuilder();
$containerBuilder->setParameter('es_url', '%env(url:ES_HOST)%');
$containerBuilder->compile(true);

var_dump($containerBuilder->getParameter('es_url'));
ES_HOST='http://www.google.com' php index.php
array(8) {
  ["scheme"]=>
  string(4) "http"
  ["host"]=>
  string(14) "www.google.com"
  ["port"]=>
  int(0)
  ["user"]=>
  string(0) ""
  ["pass"]=>
  string(0) ""
  ["path"]=>
  string(0) ""
  ["query"]=>
  string(0) ""
  ["fragment"]=>
  string(0) ""
}
@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

@jderusse thanks for the added examples. That's all we need for the docs. Cheers!

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

(rebase needed)
@symfony/deciders OK for you?

@jderusse jderusse force-pushed the jderusse:env-url branch from cdf9e79 to eae6c06 Dec 1, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

Please rebase.

@jderusse jderusse force-pushed the jderusse:env-url branch 2 times, most recently from fc01bbc to 891f1d3 Jan 27, 2019

@jderusse jderusse force-pushed the jderusse:env-url branch 4 times, most recently from ea2546d to cfb5cbc Mar 9, 2019

@jderusse jderusse force-pushed the jderusse:env-url branch from cfb5cbc to 4cf00cb Mar 9, 2019

@jderusse jderusse changed the title Add an url EnvProcessor [DI] Add an url EnvProcessor Mar 10, 2019

@jderusse jderusse force-pushed the jderusse:env-url branch from 4cf00cb to 395fc87 Mar 10, 2019

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Reading the updated description of this PR, I think this:

charset: '%env(key:charset:query_string:key:query:url:DATABASE_DSN)%'

could be easier to understand as follows:

charset: '%env(key:charset:query_string:DATABASE_DSN)%'

query_string would extract the query string from the given URL and explode it into a PHP array.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

can we try both? query string obtained from URL and provided as-is.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

@javiereguiluz I'm not sure that's possible without restricting possibilities a lot. Power comes from combining single predictable tools, and have each tool do one thing well. Having query_string be a hybrid between parse_url and parse_str means not being able to use it only as a parse_str unit. 👎 to your proposal for this reason on my side.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Here's an approach that might work: https://3v4l.org/hLlIg, i like the proposed convenience of key:charset:query_string:VAR

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

Thanks @ro0NL
@jderusse OK to implement this? Works for me (canceling my previous comment)

@jderusse jderusse force-pushed the jderusse:env-url branch from 395fc87 to b23858e Mar 17, 2019

@jderusse

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

Done

}
if ('query_string' === $prefix) {
$queryString = parse_url($env, PHP_URL_QUERY) ?? $env;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 17, 2019

Member

Should use ?: I think as parse_url can return false also

This comment has been minimized.

Copy link
@ro0NL

ro0NL Mar 18, 2019

Contributor

If the component parameter is specified, parse_url() returns a string (or an integer, in the case of PHP_URL_PORT) instead of an array. If the requested component doesn't exist within the given URL, NULL will be returned.

🤞 but i didnt spot a case which returns false when a component is set.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 18, 2019

Member

var_export(parse_url(':', PHP_URL_QUERY)); displays false :)

This comment has been minimized.

Copy link
@ro0NL

ro0NL Mar 18, 2019

Contributor

😭

@jderusse jderusse force-pushed the jderusse:env-url branch from b23858e to f253c9b Mar 17, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Thank you @jderusse.

@nicolas-grekas nicolas-grekas merged commit f253c9b into symfony:master Mar 18, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Mar 18, 2019

feature #28975 [DI] Add an url EnvProcessor (jderusse)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[DI] Add an url EnvProcessor

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | symfony/symfony-docs#11128

This PR add a new env processor `url` to convert an URL (or DSN) into an array.

The main goal is to simplify the project configuration and reduce the number of env variable when working with bundle which are not able to deal with DSN
(pick some random project in symfony/recipes-contrib: https://github.com/symfony/recipes-contrib/blob/master/facile-it/mongodb-bundle/0.6/manifest.json or https://github.com/symfony/recipes-contrib/blob/master/wouterj/eloquent-bundle/1.0/manifest.json)

```yaml
# before
MONGODB_HOST=localhost
MONGODB_PORT=27017
MONGODB_USER=
MONGODB_PASSWORD=
MONGODB_DB=symfony

mongo_db_bundle:
    data_collection: '%kernel.debug%'
    clients:
        default:
            hosts:
            - { host: '%env(MONGODB_HOST)%', port: '%env(int:MONGODB_PORT)%' }
            username: '%env(MONGODB_USER)%'
            password: '%env(MONGODB_PASSWORD)%'
            connectTimeoutMS: 3000
    connections:
        default:
            database_name: '%env(MONGODB_DB)%'

# after
MONGODB_DSN=mongodb://localhost:27017/symfony

mongo_db_bundle:
    data_collection: '%kernel.debug%'
    clients:
        default:
            hosts:
            - { host: '%env(key:host:url:MONGODB_DSN)%', port: '%env(key:port:url:MONGODB_DSN)%' }
            username: '%env(key:user:url:MONGODB_DSN)%'
            password: '%env(key:pass:url:MONGODB_DSN)%'
            connectTimeoutMS: 3000
    connections:
        default:
            database_name: '%env(key:path:url:MONGODB_DSN)%'
```

Added also a `query_string` processor to parse query string

```
DATABASE_DSN=mysql://localhost/db?charset=utf8

foo:
  bar:
    charset: '%env(key:charset:query_string🔑query:url:DATABASE_DSN)%'
```

Commits
-------

f253c9b Add an url EnvProcessor

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 25, 2019

minor #11128 Add `url` and `query_string` processors (jderusse)
This PR was merged into the master branch.

Discussion
----------

Add `url` and `query_string` processors

Documentation for symfony/symfony#28975

Commits
-------

4fb0d0a Add `url` and `query_string` processors

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.