[Config] Create XML Reference Dumper #8635

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
4 participants
@wouterj
Member

wouterj commented Aug 1, 2013

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? no
Fixed tickets -
License MIT
Doc PR -

Only Yaml was supported. This PR adds support for XML. This makes it easier to test XML schema's (see symfony-cmf/MenuBundle#114 ), helps us at the docs with our configuration reference and helps others using XML with symfony.

Todo

  • Prototyped arrays don't work properly
  • Add comments (see Yaml dumper)
  • Add namespaces support

Side effects

I've moved the reference dumpers to their own namespace and renamed the original reference dumper to YamlReferenceDumper. The old one is kept for BC, but deprecated.

/cc @dantleech

@stloyd

View changes

src/Symfony/Component/Config/Definition/Dumper/XmlReferenceDumper.php
+ }
+
+ if (is_array($value)) {
+ if (0 === count($value)) {

This comment has been minimized.

@stloyd

stloyd Aug 1, 2013

Contributor

It's useless as empty($value) will return true already.

@stloyd

stloyd Aug 1, 2013

Contributor

It's useless as empty($value) will return true already.

This comment has been minimized.

@staabm

staabm Aug 4, 2013

Contributor

Nit: will return empty string already :)

@staabm

staabm Aug 4, 2013

Contributor

Nit: will return empty string already :)

This comment has been minimized.

@wouterj

wouterj Aug 4, 2013

Member

fixedd

@wouterj

wouterj Aug 4, 2013

Member

fixedd

@stloyd

View changes

src/Symfony/Component/Config/Definition/Dumper/XmlReferenceDumper.php
+ }
+ }
+
+ $rootIsEmptyTag = (0 === count($rootChildren));

This comment has been minimized.

@stloyd

stloyd Aug 1, 2013

Contributor

No need for the brackets.

@stloyd

stloyd Aug 1, 2013

Contributor

No need for the brackets.

This comment has been minimized.

@wouterj

wouterj Aug 1, 2013

Member

It makes it a lot more readable

@wouterj

wouterj Aug 1, 2013

Member

It makes it a lot more readable

@wouterj

View changes

phpunit.xml.dist
+ <testsuite name="Config">
+ <directory>./src/Symfony/Component/Config/Tests/</directory>
+ <exclude>./src/Symfony/Component/Config/Tests/Resource/FileResourceTest.php</exclude>
+ </testsuite>

This comment has been minimized.

@wouterj

wouterj Aug 1, 2013

Member

this will be removed when the pr is ready

@wouterj

wouterj Aug 1, 2013

Member

this will be removed when the pr is ready

+
+ /**
+ * @param NodeInterface $node
+ * @param integer $depth

This comment has been minimized.

@staabm

staabm Aug 4, 2013

Contributor

Missing last param

@staabm

staabm Aug 4, 2013

Contributor

Missing last param

This comment has been minimized.

@wouterj

wouterj Aug 20, 2013

Member

fixed

@wouterj

wouterj Aug 20, 2013

Member

fixed

@stof

View changes

src/Symfony/Component/Config/Definition/ReferenceDumper.php
- * Currently, only YML format is supported.
- *
- * @author Kevin Bond <kevinbond@gmail.com>
+ * @deprecated Deprecated since version 2.4, to be removed in 3.0. Use Symfony\Component\Config\Definition\DumperYamlReferenceDumper instead.

This comment has been minimized.

@stof

stof Aug 4, 2013

Member

missing \

@stof

stof Aug 4, 2013

Member

missing \

This comment has been minimized.

@wouterj

wouterj Aug 4, 2013

Member

fixed

@wouterj

wouterj Aug 4, 2013

Member

fixed

@stof

View changes

...fony/Component/Config/Tests/Definition/Dumper/XmlReferenceDumperTest.php
+<config
+ boolean="true"
+ scalar-empty=""
+ scalar-null=""

This comment has been minimized.

@stof

stof Aug 4, 2013

Member

this is wrong. null should be dumped as "null", not as an empty string (otherwise it will be loaded as an empty string)

@stof

stof Aug 4, 2013

Member

this is wrong. null should be dumped as "null", not as an empty string (otherwise it will be loaded as an empty string)

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Aug 20, 2013

Member

I fixed the comments and added support for comments in XML.

The only thing missing now is correct prototyped arrays, but I don't know the correct format for that. Can someone confirm my format is correct? Or can someone provide a correct format?

Member

wouterj commented Aug 20, 2013

I fixed the comments and added support for comments in XML.

The only thing missing now is correct prototyped arrays, but I don't know the correct format for that. Can someone confirm my format is correct? Or can someone provide a correct format?

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Sep 9, 2013

Member

I changed the example to use a more common prototype example and fixed the dumpers.

@fabpot I think it's ready to merge now

Member

wouterj commented Sep 9, 2013

I changed the example to use a more common prototype example and fixed the dumpers.

@fabpot I think it's ready to merge now

@stof

View changes

src/Symfony/Component/Config/Definition/Dumper/XmlReferenceDumper.php
+ foreach ($rootAttributes as $attrName => $attrValue) {
+ $attr = sprintf('%s="%s"', $attrName, $this->writeValue($attrValue));
+
+ if (0 === $attributesCount) {

This comment has been minimized.

@stof

stof Sep 9, 2013

Member

It is never 0 here as you are in the else part of if (1 >= ($attributesCount = count($rootAttributes))) {, which means 1 < $attributesCount

@stof

stof Sep 9, 2013

Member

It is never 0 here as you are in the else part of if (1 >= ($attributesCount = count($rootAttributes))) {, which means 1 < $attributesCount

This comment has been minimized.

@wouterj

wouterj Sep 9, 2013

Member

fixed

@wouterj

wouterj Sep 9, 2013

Member

fixed

@stof

View changes

src/Symfony/Component/Config/Definition/Dumper/XmlReferenceDumper.php
+
+ // root node comment
+ if (isset($rootComment)) {
+ $this->writeLine('<!-- '.$rootComment.' -->', $depth);

This comment has been minimized.

@stof

stof Sep 9, 2013

Member

This is broken when you are in a prototype as it will close the comment opened for the prototype:

<!-- prototype
<!-- root comments -->
<more stuff="in the prototype comment" />
-->
@stof

stof Sep 9, 2013

Member

This is broken when you are in a prototype as it will close the comment opened for the prototype:

<!-- prototype
<!-- root comments -->
<more stuff="in the prototype comment" />
-->

This comment has been minimized.

@stof

stof Sep 9, 2013

Member

This will also break in case of nested array nodes btw.

Why putting the whole prototype in a comment ?

@stof

stof Sep 9, 2013

Member

This will also break in case of nested array nodes btw.

Why putting the whole prototype in a comment ?

This comment has been minimized.

@wouterj

wouterj Sep 9, 2013

Member

I fixed it by adding 'prototype' to the root comments

@wouterj

wouterj Sep 9, 2013

Member

I fixed it by adding 'prototype' to the root comments

@stof

View changes

...fony/Component/Config/Tests/Definition/Dumper/XmlReferenceDumperTest.php
+
+ $dumper = new XmlReferenceDumper();
+ $this->assertEquals($this->getConfigurationAsString(), $dumper->dump($configuration));
+// echo("\n\n".$dumper->dump($configuration));

This comment has been minimized.

@stof

stof Sep 9, 2013

Member

this should be removed

@stof

stof Sep 9, 2013

Member

this should be removed

This comment has been minimized.

@wouterj

wouterj Sep 9, 2013

Member

done

@stof

View changes

...fony/Component/Config/Tests/Definition/Dumper/XmlReferenceDumperTest.php
+ <array-prototype>
+
+ <!-- prototype
+ <parameter name="parameter name" />

This comment has been minimized.

@stof

stof Sep 9, 2013

Member

For scalar prototypes, it looks like <parameter name="parameter name">value</parameter>. Using a self closing tag works to configure an empty string but not for other values so it should be rendered with a closing tag IMO

@stof

stof Sep 9, 2013

Member

For scalar prototypes, it looks like <parameter name="parameter name">value</parameter>. Using a self closing tag works to configure an empty string but not for other values so it should be rendered with a closing tag IMO

This comment has been minimized.

@wouterj

wouterj Sep 9, 2013

Member

fixed

@wouterj

wouterj Sep 9, 2013

Member

fixed

@stof

View changes

...y/Component/Config/Tests/Fixtures/Configuration/ExampleConfiguration.php
@@ -48,14 +49,11 @@ public function getConfigTreeBuilder()
->end()
->end()
->arrayNode('array_prototype')

This comment has been minimized.

@stof

stof Sep 9, 2013

Member

I think it would be less confusing if this node was renamed. The prototyped array node is parameters, not array_prototype

@stof

stof Sep 9, 2013

Member

I think it would be less confusing if this node was renamed. The prototyped array node is parameters, not array_prototype

This comment has been minimized.

@stof

stof Sep 9, 2013

Member

And you should also keep an example of a prototyped node with an array as prototype (as it happens regularly too) but giving it several children in this case (a single child value is a case where the prototype should be scalar)

@stof

stof Sep 9, 2013

Member

And you should also keep an example of a prototyped node with an array as prototype (as it happens regularly too) but giving it several children in this case (a single child value is a case where the prototype should be scalar)

This comment has been minimized.

@wouterj

wouterj Sep 9, 2013

Member

fixed name, I'll need to add another tests for array prototypes

@wouterj

wouterj Sep 9, 2013

Member

fixed name, I'll need to add another tests for array prototypes

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Sep 9, 2013

Member

Thank you @stof! I've fixed most of your comments. I've also added support for enum nodes in both dumpers.

Member

wouterj commented Sep 9, 2013

Thank you @stof! I've fixed most of your comments. I've also added support for enum nodes in both dumpers.

@stof

View changes

src/Symfony/Component/Config/Definition/Dumper/XmlReferenceDumper.php
+ break;
+
+ case 'Symfony\Component\Config\Definition\EnumNode':
+ $prototypeValue = implode('|', $prototype->getValues());

This comment has been minimized.

@stof

stof Sep 9, 2013

Member

This is not be safe. It is possible to use an array as one of the possible values of the enum. EnumNode::finalizeValue uses implode(', ', array_map('json_encode', $this->values)) to display the permissible values in the exception message.

@stof

stof Sep 9, 2013

Member

This is not be safe. It is possible to use an array as one of the possible values of the enum. EnumNode::finalizeValue uses implode(', ', array_map('json_encode', $this->values)) to display the permissible values in the exception message.

This comment has been minimized.

@stof

stof Sep 9, 2013

Member

note that writing XML config files while putting an array in it can get nasty (same than putting an array in a variableNode)

@stof

stof Sep 9, 2013

Member

note that writing XML config files while putting an array in it can get nasty (same than putting an array in a variableNode)

@stof

View changes

...ony/Component/Config/Tests/Definition/Dumper/YamlReferenceDumperTest.php
- name:
- value: ~ # Required
+ # Prototype
+ name: []

This comment has been minimized.

@stof

stof Sep 9, 2013

Member

this is wrong. The prototype is a scalar, not an array. The config will look like this:

parameters:
    name: value
    foobar: true
    name2: value2
@stof

stof Sep 9, 2013

Member

this is wrong. The prototype is a scalar, not an array. The config will look like this:

parameters:
    name: value
    foobar: true
    name2: value2

This comment has been minimized.

@wouterj

wouterj Sep 10, 2013

Member

fixed (it was a bug in the yaml dumper)

@wouterj

wouterj Sep 10, 2013

Member

fixed (it was a bug in the yaml dumper)

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Sep 10, 2013

Member

I did some more stuff:

  • Added namespace support
  • Added (failing) test for array prototypes without a key
Member

wouterj commented Sep 10, 2013

I did some more stuff:

  • Added namespace support
  • Added (failing) test for array prototypes without a key

wouterj added some commits Sep 9, 2013

Moved ReferenceDumper to Dumper\YamlReferenceDumper
This is done to make other formats, such as the XmlReferenceDumper
possible. This deprecates the old
Symfony\Component\Config\Definition\ReferenceDumper.
Fixed Yaml Dumper
Fixed Yaml scalar prototypes
@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Sep 17, 2013

Member

@fabpot I think I'm done now. The only problem is rendering the prototype values in Yaml. The Dumper needs an almost complete rewrite to fix that test and that's out of scope for this PR. I've marked the test for the YamlDumper as incomplete, so we can still safely merge this PR for 2.4.

Member

wouterj commented Sep 17, 2013

@fabpot I think I'm done now. The only problem is rendering the prototype values in Yaml. The Dumper needs an almost complete rewrite to fix that test and that's out of scope for this PR. I've marked the test for the YamlDumper as incomplete, so we can still safely merge this PR for 2.4.

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Sep 17, 2013

Member

btw, the test failure is just a random symfony failure which has nothing to do with this PR

Member

wouterj commented Sep 17, 2013

btw, the test failure is just a random symfony failure which has nothing to do with this PR

@fabpot fabpot closed this in 7005cf5 Sep 18, 2013

fabpot added a commit that referenced this pull request Sep 18, 2013

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