Definitions are not supported in DI YamlDumper / YamlFileLoader #7526

Closed
wants to merge 2 commits into
from

Projects

None yet

6 participants

Contributor
chx commented Mar 29, 2013
Q A
Bug fix? [yes]
New feature? [no]
BC breaks? [no]
Deprecations? [no]
Tests pass? [yes]
Fixed tickets []
License MIT
Doc PR []
Contributor
igorw commented Mar 29, 2013

Supporting inline definitions for YAML is definitely quite an interesting addition. What is lacking from this patch IMO, is support for arguments and ideally all the other Definition attributes.

@stof stof commented on the diff Apr 6, 2013
...mponent/DependencyInjection/Loader/YamlFileLoader.php
@@ -315,6 +315,8 @@ private function resolveServices($value)
if (null !== $invalidBehavior) {
$value = new Reference($value, $invalidBehavior, $strict);
}
+ } elseif (is_string($value) && 0 === strpos($value, '+')) {
+ $value = new Definition(substr($value, 1));
stof
stof Apr 6, 2013 Member

This is wrong. the inline definition should either be fully supported (i.e. a full service definition, which cannot be done with a string) or not at all (the current state). But supporting only the class name without any of the settings is a no-go (and would still make the dumper wrong as dumping and loading would not recreate the same definition)

chx
chx Apr 6, 2013 Contributor

I already discussed this with igorw and we agreed we need a more complete
dumper. I have some code available, I will push it to somewhere, but it's
useless -- we need to think through first what format of the Definition
dump should take. Should it be inlined? If not, then should it change the
parent into a non-inlined representation as well?

On Fri, Apr 5, 2013 at 5:37 PM, Christophe Coevoet <notifications@github.com

wrote:

In src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php:

@@ -315,6 +315,8 @@ private function resolveServices($value)
if (null !== $invalidBehavior) {
$value = new Reference($value, $invalidBehavior, $strict);
}

  •    } elseif (is_string($value) &&  0 === strpos($value, '+')) {
    
  •        $value = new Definition(substr($value, 1));
    

This is wrong. the inline definition should either be fully supported
(i.e. a full service definition, which cannot be done with a string) or not
at all (the current state). But supporting only the class name without any
of the settings is a no-go (and would still make the dumper wrong as
dumping and loading would not recreate the same definition)


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/pull/7526/files#r3682709
.

@pborreli pborreli commented on the diff Apr 6, 2013
...y/Component/DependencyInjection/Dumper/YamlDumper.php
@@ -236,7 +246,7 @@ private function dumpValue($value)
*/
private function getServiceCall($id, Reference $reference = null)
{
- if (null !== $reference && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $reference->getInvalidBehavior()) {
+ if (NULL !== $reference && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $reference->getInvalidBehavior()) {
pborreli
pborreli Apr 6, 2013 Contributor

keep it lowercase please

@staabm staabm commented on the diff Apr 21, 2013
...y/Component/DependencyInjection/Dumper/YamlDumper.php
@@ -64,14 +66,18 @@ public function dump(array $options = array())
*
* @param string $id
* @param Definition $definition
+ * @param string $prefix
staabm
staabm Apr 21, 2013 Contributor

Int

Contributor
chx commented Apr 21, 2013

Note that I do not plan to pursue this further. This code fires the recursion from dumpValue and that ends up with escaped \n characters and such. Totally unreadable. Somehow dumpValue needs to know when not to inline... it's a mess.

Owner
fabpot commented Apr 21, 2013

Closing this PR then.

@fabpot fabpot closed this Apr 21, 2013
Contributor
chx commented Apr 21, 2013

Should I change this to a bug report then? Someone should pursue it even if I don't...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment