Skip to content

[FrameworkBundle] allow container/routing configurators to vary by env #40214

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

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 16, 2021

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets #40215
License MIT
Doc PR -

Inspired by symfony/webpack-encore#900 and by a chat on Slack with @weaverryan

This aims at allowing conditional configuration, which would allow merging config files in one.

Using the PHP-DSL:

$container
    ->when(env: 'prod')
    ->services()
        ->set(Foo::class)
    //...

In Yaml:

framework:
    secret: '%env(APP_SECRET)%'

when@dev:
    services:
        App\FooForDev: ~

when@test:
    framework:
        test: true
        session:
            storage_factory_id: session.storage.mock_file

In XML (omitting namespaces):

<when env="test">
	<framework test="true">
		<!-- ... -->
	</framework>
</when>

A similar syntax is also provided for routes, with support for annotations:
@Route(env="prod") defines a route that is enabled only on the "prod" env.

@carsonbot
Copy link

Hey!

Well done!, Im impressed by this PR.

I think @jschaedl has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@nicolas-grekas nicolas-grekas force-pushed the conf-vary-by-env branch 4 times, most recently from d7c1696 to 1f44487 Compare February 17, 2021 17:50
@nicolas-grekas nicolas-grekas changed the title [FrameworkBundle] allow container/routing configurators to vary by env/debug [FrameworkBundle] allow container/routing configurators to vary by env Feb 17, 2021
@wouterj
Copy link
Member

wouterj commented Feb 17, 2021

I'm wondering how this works with @ being reserved for future use in Yaml?

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Feb 17, 2021

I'm wondering how this works with @ being reserved for future use in Yaml?

🤦

Then we might do when@dev, or when-env@dev, or env@dev, etc. Any preference?

@nicolas-grekas nicolas-grekas force-pushed the conf-vary-by-env branch 2 times, most recently from 5c9641b to aafba5e Compare February 17, 2021 23:50
@wouterj
Copy link
Member

wouterj commented Feb 17, 2021

Maybe we can use Yaml tags as key:

!when test:
  framework: 
    test: true

(Not sure if this requires the ? complex mapping key indicator:

? !when test:
  framework: 
    test: true

)

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Feb 17, 2021

We (PHP) don't support tagged (complex) keys :'(

@nicolas-grekas nicolas-grekas force-pushed the conf-vary-by-env branch 3 times, most recently from da1cefb to ebe73b6 Compare February 18, 2021 10:12
@nicolas-grekas
Copy link
Member Author

PR is ready, with tests (failures are false positives).

All types of loaders now handle the $env: annotations, object loaders, the various file loaders, etc.

@stof
Copy link
Member

stof commented Feb 18, 2021

I'm wondering how this works with @ being reserved for future use in Yaml?

it would require quoting the key

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Reviewed! I love this - it solves my last favorite thing about YAML currently: the fact that we need to split up files for tiny environment differences (the WebpackEncoreBundle recipe has 3 config files... 2 of them are for the test/prod env and are nearly empty).

👍

@ogizanagi
Copy link
Contributor

Great! What about debug (APP_DEBUG) as well?

$container
    ->when(debug: true)

@nicolas-grekas
Copy link
Member Author

I've thought about that, and I figured out that no recipes vary by "debug", and I never felt the need to do so.
That's why I'm not proposing it.

@ogizanagi
Copy link
Contributor

That's true for recipes indeed, since bundles usually handle this logic in their extension / already have configuration keys for specific debug features, which can be easily toggled using %kernel.debug%.

Yet, it happens from times to times to have debug services & decorators in userland code that could benefit from this.
Not much a blocker here, but we should just anticipate this need regarding the YAML format which might be a bit limiting as-is.

@nicolas-grekas
Copy link
Member Author

Not much a blocker here, but we should just anticipate this need regarding the YAML format which might be a bit limiting as-is.

I think there will always be a way, eg when@debug or when+debug, etc. But given that the debug flag is auto-set in dev and auto unset in prod, the env is almost always enough. Ppl that really need to vary by debug could either use a bundle, or use some logic in their Kernel, or contribute a PR here ;)

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Feb 22, 2021

Thank you all for the review!

@ogizanagi
Copy link
Contributor

Fair enough, thx for this feature ❤️

@Nyholm
Copy link
Member

Nyholm commented Apr 14, 2021

Please note that #40808 will revert the changes in the PHP config.

nicolas-grekas added a commit that referenced this pull request Apr 14, 2021
… config (Nyholm)

This PR was merged into the 5.3-dev branch.

Discussion
----------

[DependencyInjection][Routing] Access environment in PHP config

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? |
| Tickets       |
| License       | MIT
| Doc PR        |

This will allow me to write config like:

```php
use Symfony\Config\FrameworkConfig;
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;

return static function (FrameworkConfig $framework, ContainerConfigurator $container) {
    if ('test' === $container->env()) {
        $framework->test(true);
        $framework->session()->storageFactoryId('session.storage.mock_file');
    }
};
```

This PR will also revert parts of #40214. It is much simpler to maintain and write PHP config without `->when()`. Instead we add `ContainerConfigurator::env(): ?string` and `RoutingConfigurator::env(): ?string`.

```php

use App\Controller\BlogController;
use Symfony\Component\Routing\Loader\Configurator\RoutingConfigurator;

return function (RoutingConfigurator $routes) {
    if ($routes->env() === 'dev') {
        $routes->add('debug-stats', '/blog-debyg')
            ->controller([BlogController::class, 'debug'])
        ;
    }

    $routes->add('blog_list', '/blog')
        ->controller([BlogController::class, 'list'])
    ;
};
```

Commits
-------

11dfaa4 [DependencyInjection][Routing] Access environment in PHP config
nicolas-grekas added a commit that referenced this pull request Apr 17, 2021
…p autoregistering a class when the env doesn't match (nicolas-grekas)

This PR was merged into the 5.3-dev branch.

Discussion
----------

[DependencyInjection] Add `#[When(env: 'foo')]` to skip autoregistering a class when the env doesn't match

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

This is a follow up of #40214, in order to conditionally auto-register classes.

By adding a `#[When(env: prod)]` annotation on a class, one can tell that a class should be skipped when the current env doesn't match the one declared in the attribute.

This saves from writing similar conditional configuration by using the per-env `services_prod.yaml` convention (+corresponding exclusion from `services.yaml`), or some logic in the Kernel.

Commits
-------

59c75ba [DI] add `#[When(env: 'foo')]` to skip autoregistering a class when the env doesn't match
@fabpot fabpot mentioned this pull request Apr 18, 2021
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.

9 participants