Skip to content

Commit

Permalink
feature #21196 [FrameworkBundle] changed some default configs from ca…
Browse files Browse the repository at this point in the history
…nBeEnabled to canBeDisabled (fabpot)

This PR was squashed before being merged into the 3.3-dev branch (closes #21196).

Discussion
----------

[FrameworkBundle] changed some default configs from canBeEnabled to canBeDisabled

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

FrameworkBundle configuration is currently "optimized" for when a project
depends on symfony/symfony (which is the vast majority of Symfony projects out
there as that's how Symfony Standard Edition is set up). As all components are
always available, features (forms, validation, translation, serializer, ...)
are disabled by default in FrameworkBundle's configuration (`canBeEnabled`) and
developers must enable them when they need them (that was done mainly for
performance reasons).

That's annoying as it means one configuration step before being able to use
forms for the first time (or validation, or serialization, or translation, ...).

To make features auto-configurable and make the framework a bit more
user-friendly (that's where I think I need to invoke DX :), I'd like Symfony 4
to work in a different way. Instead of relying on symfony/symfony, I want
people to install only the components/bundles they need. In that scenario, we
can auto-configure Symfony FrameworkBundle based on the available components.
If you add symfony/form as a dependency, then it makes sense to automatically
enable the feature in framework bundle (with the possibility to disable it if
needed thanks to `canBeDisabled`).

Let's recap:

 * Before:

   * You want to use forms; you have symfony/symfony, so nothing to install;

   * Using forms does not work out of the box though; you need to know that you
     have to edit `app/config/config.yml` to explicitly enable forms;

   * Forms work!

 * After:

   * You want to use forms so you install symfony/form (like for any other
     packages out there; want to use Twig for templating, install twig/twig);

   * But for Symfony components, there are no other steps; forms are
     auto-configured just because you installed the dependency, go work now!

In a way, it makes handling/installing/configuring Symfony components no
different than doing the same for a third party package. That's about relying
even more on Composer and less on configuration. Symfony components have the
extra benefit of being auto-configured via FrameworkBundle. That's not the case
for other third-party packages/bundles, but for those who attended SymfonyCon
Berlin, you know that this is coming soon via Symfony Flex.

That's even more interesting for forms as CSRF protection needs an extra knob
to be turned on currently. With the new way, just install the CSRF security
component. An again, you still have the possibility to turn it off if you want
to.

Anyway, this PR gives us the flexibility to do both: when using
symfony/symfony, everything works as before, if you are using
symfony/framework-bundle, then auto-configuration based on the installed
packages is automatically activated.

This also brings consistency as this behavior is already what we've done for
the Doctrine Annotation library in 3.2.

Last, but not the least, with all the work currently done on the container
lazyness for Symfony 3.3, concerns about performance are less important than
before, so having components auto-enabled when installed should not be a big
deal. We might even go one step further and remove enabling/disabling for
ESI/SSI/fragments/... And whenever we create an independent Session component,
we will be able to do the same with the session configuration. The astute
reader might have noticed that I haven't talked about the templating
configuration, but as the component will be deprecated in 3.3, I prefer to keep
its activation explicit.

In terms of BC, the only change is for people using symfony/framework-bundle
with some packages that were installed but not enabled in the config. I would
say that this should be pretty rare and anyway, the only consequence is a small
performance hit which can be easily offset by explicitly disabling the config.

That's all folks!

Commits
-------

ef80873 [FrameworkBundle] changed some default configs from canBeEnabled to canBeDisabled
98ce21a [FrameworkBundle] changed the default value of annotation setting based on the existence of Doctrine Annotations
  • Loading branch information
fabpot committed Jan 8, 2017
2 parents e1d3900 + ef80873 commit 8e497f2
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 23 deletions.
3 changes: 3 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
Expand Up @@ -4,6 +4,9 @@ CHANGELOG
3.3.0
-----

* Changed default configuration for
assets/forms/validation/translation/serialization/csrf from `canBeEnabled()` to
`canBeDisabled()` when Flex is used
* The server:* commands and their associated router files were moved to WebServerBundle
* Translation related services are not loaded anymore when the `framework.translator` option
is disabled.
Expand Down
Expand Up @@ -12,9 +12,15 @@
namespace Symfony\Bundle\FrameworkBundle\DependencyInjection;

use Doctrine\Common\Annotations\Annotation;
use Symfony\Bundle\FullStack;
use Symfony\Component\Asset\Package;
use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
use Symfony\Component\Config\Definition\Builder\TreeBuilder;
use Symfony\Component\Config\Definition\ConfigurationInterface;
use Symfony\Component\Form\Form;
use Symfony\Component\Serializer\Serializer;
use Symfony\Component\Translation\Translator;
use Symfony\Component\Validator\Validation;

/**
* FrameworkExtension configuration structure.
Expand Down Expand Up @@ -139,7 +145,7 @@ private function addFormSection(ArrayNodeDefinition $rootNode)
->children()
->arrayNode('form')
->info('form configuration')
->canBeEnabled()
->{!class_exists(FullStack::class) && class_exists(Form::class) ? 'canBeDisabled' : 'canBeEnabled'}()
->children()
->arrayNode('csrf_protection')
->treatFalseLike(array('enabled' => false))
Expand Down Expand Up @@ -506,7 +512,7 @@ private function addAssetsSection(ArrayNodeDefinition $rootNode)
->children()
->arrayNode('assets')
->info('assets configuration')
->canBeEnabled()
->{!class_exists(FullStack::class) && class_exists(Package::class) ? 'canBeDisabled' : 'canBeEnabled'}()
->fixXmlConfig('base_url')
->children()
->scalarNode('version_strategy')->defaultNull()->end()
Expand Down Expand Up @@ -573,7 +579,7 @@ private function addTranslatorSection(ArrayNodeDefinition $rootNode)
->children()
->arrayNode('translator')
->info('translator configuration')
->canBeEnabled()
->{!class_exists(FullStack::class) && class_exists(Translator::class) ? 'canBeDisabled' : 'canBeEnabled'}()
->fixXmlConfig('fallback')
->fixXmlConfig('path')
->children()
Expand All @@ -598,10 +604,10 @@ private function addValidationSection(ArrayNodeDefinition $rootNode)
->children()
->arrayNode('validation')
->info('validation configuration')
->canBeEnabled()
->{!class_exists(FullStack::class) && class_exists(Validation::class) ? 'canBeDisabled' : 'canBeEnabled'}()
->children()
->scalarNode('cache')->end()
->booleanNode('enable_annotations')->defaultFalse()->end()
->booleanNode('enable_annotations')->{!class_exists(FullStack::class) && class_exists(Annotation::class) ? 'defaultTrue' : 'defaultFalse'}()->end()
->arrayNode('static_method')
->defaultValue(array('loadValidatorMetadata'))
->prototype('scalar')->end()
Expand Down Expand Up @@ -642,9 +648,9 @@ private function addSerializerSection(ArrayNodeDefinition $rootNode)
->children()
->arrayNode('serializer')
->info('serializer configuration')
->canBeEnabled()
->{!class_exists(FullStack::class) && class_exists(Serializer::class) ? 'canBeDisabled' : 'canBeEnabled'}()
->children()
->booleanNode('enable_annotations')->defaultFalse()->end()
->booleanNode('enable_annotations')->{!class_exists(FullStack::class) && class_exists(Annotation::class) ? 'defaultTrue' : 'defaultFalse'}()->end()
->scalarNode('cache')->end()
->scalarNode('name_converter')->end()
->end()
Expand Down
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection;

use Symfony\Bundle\FrameworkBundle\DependencyInjection\Configuration;
use Symfony\Bundle\FullStack;
use Symfony\Component\Config\Definition\Processor;

class ConfigurationTest extends \PHPUnit_Framework_TestCase
Expand Down Expand Up @@ -176,7 +177,7 @@ protected static function getBundleDefaultConfig()
'enabled' => false,
),
'form' => array(
'enabled' => false,
'enabled' => !class_exists(FullStack::class),
'csrf_protection' => array(
'enabled' => null, // defaults to csrf_protection.enabled
'field_name' => '_token',
Expand All @@ -200,14 +201,14 @@ protected static function getBundleDefaultConfig()
),
),
'translator' => array(
'enabled' => false,
'enabled' => !class_exists(FullStack::class),
'fallbacks' => array('en'),
'logging' => true,
'paths' => array(),
),
'validation' => array(
'enabled' => false,
'enable_annotations' => false,
'enabled' => !class_exists(FullStack::class),
'enable_annotations' => !class_exists(FullStack::class),
'static_method' => array('loadValidatorMetadata'),
'translation_domain' => 'validators',
'strict_email' => false,
Expand All @@ -219,8 +220,8 @@ protected static function getBundleDefaultConfig()
'enabled' => true,
),
'serializer' => array(
'enabled' => false,
'enable_annotations' => false,
'enabled' => !class_exists(FullStack::class),
'enable_annotations' => !class_exists(FullStack::class),
),
'property_access' => array(
'magic_call' => false,
Expand Down Expand Up @@ -258,7 +259,7 @@ protected static function getBundleDefaultConfig()
'loaders' => array(),
),
'assets' => array(
'enabled' => false,
'enabled' => !class_exists(FullStack::class),
'version_strategy' => null,
'version' => null,
'version_format' => '%%s?%%s',
Expand Down
Expand Up @@ -11,6 +11,8 @@

namespace Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection;

use Doctrine\Common\Annotations\Annotation;
use Symfony\Bundle\FullStack;
use Symfony\Bundle\FrameworkBundle\Tests\TestCase;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\FrameworkExtension;
use Symfony\Component\Cache\Adapter\ApcuAdapter;
Expand All @@ -24,6 +26,7 @@
use Symfony\Component\DependencyInjection\Loader\ClosureLoader;
use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\Serializer\Serializer;
use Symfony\Component\Serializer\Mapping\Factory\CacheClassMetadataFactory;
use Symfony\Component\Serializer\Mapping\Loader\XmlFileLoader;
use Symfony\Component\Serializer\Mapping\Loader\YamlFileLoader;
Expand Down Expand Up @@ -413,7 +416,9 @@ public function testValidation()

$calls = $container->getDefinition('validator.builder')->getMethodCalls();

$this->assertCount(6, $calls);
$annotations = !class_exists(FullStack::class) && class_exists(Annotation::class);

$this->assertCount($annotations ? 7 : 6, $calls);
$this->assertSame('setConstraintValidatorFactory', $calls[0][0]);
$this->assertEquals(array(new Reference('validator.validator_factory')), $calls[0][1]);
$this->assertSame('setTranslator', $calls[1][0]);
Expand All @@ -422,10 +427,14 @@ public function testValidation()
$this->assertSame(array('%validator.translation_domain%'), $calls[2][1]);
$this->assertSame('addXmlMappings', $calls[3][0]);
$this->assertSame(array($xmlMappings), $calls[3][1]);
$this->assertSame('addMethodMapping', $calls[4][0]);
$this->assertSame(array('loadValidatorMetadata'), $calls[4][1]);
$this->assertSame('setMetadataCache', $calls[5][0]);
$this->assertEquals(array(new Reference('validator.mapping.cache.symfony')), $calls[5][1]);
$i = 3;
if ($annotations) {
$this->assertSame('enableAnnotationMapping', $calls[++$i][0]);
}
$this->assertSame('addMethodMapping', $calls[++$i][0]);
$this->assertSame(array('loadValidatorMetadata'), $calls[$i][1]);
$this->assertSame('setMetadataCache', $calls[++$i][0]);
$this->assertEquals(array(new Reference('validator.mapping.cache.symfony')), $calls[$i][1]);
}

public function testValidationService()
Expand Down Expand Up @@ -536,10 +545,16 @@ public function testValidationNoStaticMethod()

$calls = $container->getDefinition('validator.builder')->getMethodCalls();

$this->assertCount(5, $calls);
$annotations = !class_exists(FullStack::class) && class_exists(Annotation::class);

$this->assertCount($annotations ? 6 : 5, $calls);
$this->assertSame('addXmlMappings', $calls[3][0]);
$this->assertSame('setMetadataCache', $calls[4][0]);
$this->assertEquals(array(new Reference('validator.mapping.cache.symfony')), $calls[4][1]);
$i = 3;
if ($annotations) {
$this->assertSame('enableAnnotationMapping', $calls[++$i][0]);
}
$this->assertSame('setMetadataCache', $calls[++$i][0]);
$this->assertEquals(array(new Reference('validator.mapping.cache.symfony')), $calls[$i][1]);
// no cache, no annotations, no static methods
}

Expand Down Expand Up @@ -572,7 +587,7 @@ public function testStopwatchEnabledWithDebugModeDisabled()
public function testSerializerDisabled()
{
$container = $this->createContainerFromFile('default_config');
$this->assertFalse($container->has('serializer'));
$this->assertSame(!class_exists(FullStack::class) && class_exists(Serializer::class), $container->has('serializer'));
}

public function testSerializerEnabled()
Expand Down
21 changes: 21 additions & 0 deletions src/Symfony/Bundle/FullStack.php
@@ -0,0 +1,21 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bundle;

/**
* A marker to be able to check if symfony/symfony is installed instead of the individual components/bundles.
*
* @internal
*/
final class FullStack
{
}

0 comments on commit 8e497f2

Please sign in to comment.