8000 [DependencyInjection] Make YamlFileLoader raise a deprecation notice if a service definition contains unsupported keywords. by hhamon · Pull Request #17133 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DependencyInjection] Make YamlFileLoader raise a deprecation notice if a service definition contains unsupported keywords. #17133

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

Merged
merged 1 commit into from
Jan 7, 2016
Merged

[DependencyInjection] Make YamlFileLoader raise a deprecation notice if a service definition contains unsupported keywords. #17133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,30 @@
*/
class YamlFileLoader extends FileLoader
{
private static $keywords = array(
'alias' => 'alias',
'parent' => 'parent',
Copy link
Contributor

Choose a reason for hiding this comment

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

What about removing this array values as the code only uses its keys?
Ping @dunglas

Copy link
Member

Choose a reason for hiding this comment

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

values are used when constructing the message (see implode)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops didn't see that ☺️

'class' => 'class',
'shared' => 'shared',
'synthetic' => 'synthetic',
'lazy' => 'lazy',
'public' => 'public',
'abstract' => 'abstract',
'deprecated' => 'deprecated',
'factory' => 'factory',
'file' => 'file',
'arguments' => 'arguments',
'properties' => 'properties',
'configurator' => 'configurator',
'calls' => 'calls',
'tags' => 'tags',
'decorates' => 'decorates',
'decoration_inner_name' => 'decoration_inner_name',
'decoration_priority' => 'decoration_priority',
'autowire' => 'autowire',
'autowiring_types' => 'autowiring_types',
);

private $yamlParser;

/**
Expand Down Expand Up @@ -147,6 +171,8 @@ private function parseDefinition($id, $service, $file)
throw new InvalidArgumentException(sprintf('A service definition must be an array or a string starting with "@" but %s found for service "%s" in %s. Check your YAML syntax.', gettype($service), $id, $file));
}

static::checkDefinition($id, $service, $file);

if (isset($service['alias'])) {
$public = !array_key_exists('public', $service) || (bool) $service['public'];
$this->container->setAlias($id, new Alias($service['alias'], $public));
Expand Down Expand Up @@ -432,4 +458,24 @@ private function loadFromExtensions($content)
$this->container->loadFromExtension($namespace, $values);
}
}

/**
* Checks the keywords used to define a service.
*
* @param string $id The service name
* @param array $definition The service definition to check
* @param string $file The loaded YAML file
*/
private static function checkDefinition($id, array $definition, $file)
{
foreach ($definition as $key => $value) {
if (!isset(static::$keywords[$key])) {
@trigger_error(sprintf('The configuration key "%s" is unsupported for service definition "%s" in "%s". Allowed configuration keys are "%s". The YamlFileLoader object will raise an exception instead in Symfony 4.0 when detecting an unsupported service configuration key.', $key, $id, $file, implode('", "', static::$keywords)), E_USER_DEPRECATED);
// @deprecated Uncomment the following statement in Symfony 4.0
// and also update the corresponding unit test to make it expect
// an InvalidArgumentException exception.
//throw new InvalidArgumentException(sprintf('The configuration key "%s" is unsupported for service definition "%s" in "%s". Allowed configuration keys are "%s".', $key, $id, $file, implode('", "', static::$keywords)));
Copy link
Member

Choose a reason for hiding this comment

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

What about creating the 4.0 branch now to avoid having to make such updates later?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 would avoid to forget some deprecations/updates of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm 100% for creating for 4.0 branch already if @fabpot agrees.

Copy link
Member

Choose a reason for hiding this comment

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

As one of the happy few doing the merges from 2.3 up to master, I'm 👎. This is tedious enough to not add one more branch. I hope you understand...

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicolas-grekas this branch would not need to have merges as often as the others and should also not receive as much commits

Copy link
Member

Choose a reason for hiding this comment

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

The drawback would be that we would have to maintain it already, and people would start using it in their composer projects. So I'm -1 on it.

When we start the 4.0 development, we will look at any existing @trigger_error to convert them to the new code (either removing code, or doing other actions).

I would even remove the commented exception and just put a comment saying it should be converted to an exception in 4.0. This way, the exception message will be written at the 4.0 time, not now, and so using the uptodate deprecation message.

Copy link
Member

Choose a reason for hiding this comment

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

Thus, merges into newer major versions are the worse branch merges, as they often trigger merge conflicts (due to the removal of BC layers), so it is better to avoid doing them during the whole 3.x lifetime.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and creating 4.0 would require creating 3.1, 3.2, 3.3 and 3.4 also, because that's how we merge things. 👎 👎 👎

}
}
}
}
10 changes: 10 additions & 0 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/legacy_invalid_definition.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
services:
# This definition is valid and should not raise any deprecation notice
foo:
class: stdClass
arguments: [ 'foo', 'bar' ]

# This definition is invalid and must raise a deprecation notice
bar:
class: stdClass
private: true # the "private" keyword is invalid
10 changes: 10 additions & 0 deletions src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php
628C View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,4 +293,14 @@ public function testAutowire()

$this->assertTrue($container->getDefinition('bar_service')->isAutowired());
}

/**
* @group legacy
*/
public function testServiceDefinitionContainsUnsupportedKeywords()
{
$container = new ContainerBuilder();
$loader = new YamlFileLoader($container, new FileLocator(self::$fixturesPath.'/yaml'));
$loader->load('legacy_invalid_definition.yml');
}
}
0