Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[Routing] support for array values in route defaults #11394

Merged
merged 1 commit into from Jun 23, 2016

Conversation

Projects
None yet
8 participants
Member

xabbuh commented Jul 15, 2014

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

As pointed out in symfony/symfony-docs#4017, the XmlFileLoader was not capable of defining array default values.

  • array values
  • integer values
  • float values
  • boolean

@xabbuh xabbuh referenced this pull request in symfony/symfony-docs Jul 15, 2014

Merged

Clarify that route defaults don't need a placeholder #4017

Member

Tobion commented Jul 15, 2014

We would also need to find a way to represent other datatypes in xml like integer, float and boolean. Only supporting array without the other datatypes would be arbitrary.
Also possibly DateTime etc.

Member

xabbuh commented Jul 15, 2014

@Tobion Do you think that both array types and different scalar types should be done in the same pull request?

Member

Tobion commented Jul 15, 2014

Yes because if we don't find a solution for all of them, I'd argue it's not worth implementing only one part.
Then we could just say, xml only supports flat structure with strings.

Member

xabbuh commented Jul 16, 2014

I don't fully agree. Even if you were not able to use integer, float or boolean values natively in XML, you would at least be able to retain the array structure. Scalar types can be casted in the controller. Achieving the same with arrays is not always that easy (at least with nested arrays).

But I also don't think that it is too hard adding support for the other data types. So I will look into this tonight.

Member

stof commented Jul 16, 2014

@Tobion the XML loading in Symfony already converts values to boolean or number types when they look like that in the DI component. However, this is not applied in the routing loader (and applying it could be considered a BC break btw).

However, this way to represent arrays looks good (a bit painful if you want to represent lists as you have to map the keys explicitly though)

Member

xabbuh commented Jul 16, 2014

@Tobion the XML loading in Symfony already converts values to boolean or number types when they look like that in the DI component. However, this is not applied in the routing loader (and applying it could be considered a BC break btw).

@stof I thought about adding an optional type attribute to the default element. If present, the content will be casted accordingly. Otherwise it is treated as a string. This way we can maintain backward compatibility.

However, this way to represent arrays looks good (a bit painful if you want to represent lists as you have to map the keys explicitly though)

We can make the key attribute optional. This should work and shouldn't cause any BC issue. What do you think?

Member

Tobion commented Jul 16, 2014

We can make the key attribute optional. This should work and shouldn't cause any BC issue. What do you think?

The problem is that the first level needs a string key name. Otherwise you would have numeric defaults which won't work (because they get removed since preg_match also adds them).

Member

Tobion commented Jul 16, 2014

I thought about adding an optional type attribute to the default element. If present, the content will be casted accordingly.

This will not support XSD validation with types, can it? So you could add a string within a type=integer and XSD cannot raise an error.

Member

xabbuh commented Jul 16, 2014

The problem is that the first level needs a string key name. Otherwise you would have numeric defaults which won't work (because they get removed since preg_match also adds them).

Should be possible to validate that in the XSD.

This will not support XSD validation with types, can it? So you could add a string within a type=integer and XSD cannot raise an error.

No, unfortunately not. If we want that, we'll have to use different XML elements for each data type I guess.

Another possibility I see is to introduce a new attribute like infer-type which defaults to false. When this is set to true, the loader can try to infer the data type from the actual value instead of simply assuming a string value.

Member

xabbuh commented Jul 16, 2014

I think a solution that fully supports the mentioned data types both in the XML schema and in PHP and which is backward compatible requires some more work.

What I would suggest for the moment, is creating a new defaults (the name may need to be discussed) element on the same level as the current default elements. Inside it we would nest collection, string, double, integer and boolean elements. Keys can be defined the same way as before with the key attribute. To allow numerical indices, this attribute would be optional on all but the top level of the tree.

An example configuration may then look like this:

<defaults>
  <string key="foo">bar</string>
  <collection key="list">
    <boolean>true</boolean>
    <boolean>false</boolean>
    <collection>
      <string key="foobar">hash value</string>
    </collection>
  </collection>
  <integer key="baz">10</integer>
</defaults>

The process defaults would then be equal to this array:

$defaults = array(
    'foo' => 'bar',
    'list' => array(
        true,
        false,
        array('foobar' => 'hash value'),
    ),
    'baz' => 10,
);

The already existing default element would be kept with the current behaviour for backward compatibility reasons.

What do you think?

Member

stof commented Jul 17, 2014

@xabbuh using a single <defaults> instead of multiple default elements would be totally inconsistent with the way other places look in Symfony XML files. for me, it is a -1.

Member

xabbuh commented Jul 17, 2014

Staying with multiple default elements would mean something like this (the attribute should then be forbidden on the top level default child elements):

<default key="foo">
  <string>bar</string>
</default>
<default key="list">
  <collection>
    <boolean>true</boolean>
    <boolean>false</boolean>
    <collection>
      <string key="foobar">hash value</string>
    </collection>
  </collection>
</default>
<default key="baz">
  <integer>10</integer>
</default>

For backward compatibility,

<default key="foo">
  <string>bar</string>
</default>

and

<default key="foo">bar</default>

would be equivalent.

Member

Tobion commented Jul 17, 2014

The last proposal was also what I had in mind. But I would propose to distinguish between arrays (as data structure, not PHPs implementation) and maps/associative arrays. So you can validate in XSD that elements inside maps require a key and inside arrays must not have a key.
Otherwise you could mix elements with key and without inside collections which is possible in PHP but rather magical in general and not needed here. This would also prevent accidential user errors.

Member

xabbuh commented Jul 17, 2014

That's a good point since mixing list and maps also isn't possible with YAML. So, we can simply use a list and a map element.

If we agree on this, I will update the pull request to match this structure.

Member

Tobion commented Jul 17, 2014

Yeah I agree with list and map. In php the list will still be an array that allows random access, so is not really a list. But that can be considered implementation detail and semantically people usually want to express a list a values in XML when not using an associative array/map.

@xabbuh xabbuh commented on the diff Jul 18, 2014

src/Symfony/Component/Routing/Loader/XmlFileLoader.php
@@ -217,14 +217,13 @@ private function parseConfigs(\DOMElement $node, $path)
$condition = null;
foreach ($node->getElementsByTagNameNS(self::NAMESPACE_URI, '*') as $n) {
@xabbuh

xabbuh Jul 18, 2014

Member

Does anyone of you know if there was a special reason to use getElementsByTagNameNs() instead of iterating over the DOMNodeList in $node->childNodes?

@Tobion

Tobion Jul 19, 2014

Member

to support namespaces

@xabbuh

xabbuh Jul 19, 2014

Member

Isn't that covered by the elements from the childNodes too? It could then need some check for the namespaces afterwards but doesn't require to scan the complete subtree.

@Tobion

Tobion Jul 19, 2014

Member

The idea was to ignore elements from another namespace and not throw an exception for unknown tag. So one could extend the loader and xsd to support custom elements, like in FosRestBundle.
I think this won't work with childNodes because it also returns element from other namespaces within the current node.

@xabbuh

xabbuh Jul 19, 2014

Member

But throwing an exception contradicts this, doesn't it?

By the way, the new method just test for the elements local name. Should I add an additional check for the element's namespace?

@Tobion

Tobion Jul 19, 2014

Member

only throw for unkown elements in same namespace

@xabbuh

xabbuh Jul 20, 2014

Member

Sorry, of course you're right. I updated the new code to also ignore elements with a different namespace.

Member

xabbuh commented Jul 18, 2014

I updated the code to support the several data types.

@Tobion Tobion commented on an outdated diff Jul 19, 2014

.../Component/Routing/Tests/Loader/XmlFileLoaderTest.php
+ $this->assertEquals(
+ array(
+ '_controller' => 'AcmeBlogBundle:Blog:index',
+ 'values' => array(array(true, 1, 3.5, 'foo')),
+ ),
+ $route->getDefaults()
+ );
+ }
+
+ public function testListInMapDefaults()
+ {
+ $loader = new XmlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures')));
+ $routeCollection = $loader->load('list_in_map_defaults.xml');
+ $route = $routeCollection->get('blog');
+
+ $this->assertEquals(
@Tobion

Tobion Jul 19, 2014

Member

please use assertSame everywhere, so the type and order is checked

@Tobion Tobion commented on an outdated diff Jul 19, 2014

src/Symfony/Component/Routing/Loader/XmlFileLoader.php
@@ -242,4 +241,81 @@ private function parseConfigs(\DOMElement $node, $path)
return array($defaults, $requirements, $options, $condition);
}
+
+ /**
+ * Parses the "default" elements.
+ *
+ * @param \DOMElement $element The "default" element to parse
+ * @param string $path Full path of the XML file being processed
+ *
+ * @return array|bool|float|int|string The parsed value of the "default" element

@Tobion Tobion commented on an outdated diff Jul 19, 2014

src/Symfony/Component/Routing/Loader/XmlFileLoader.php
+ private function parseDefaultsConfig(\DOMElement $element, $path)
+ {
+ if ($element->hasAttribute('xsi:nil') && 'true' == $element->getAttribute('xsi:nil')) {
+ return;
+ }
+
+ // Check for existing element nodes in the default element. There can
+ // only be a single element inside a default element. So this element
+ // (if one was found) can savely be returned.
+ foreach ($element->childNodes as $child) {
+ if ($child instanceof \DOMElement) {
+ return $this->parseDefaultNode($child, $path);
+ }
+ }
+
+ // For backward compatibility: If the default element doesn't contain
@Tobion

Tobion Jul 19, 2014

Member

It's a feature, not only for BC. So you can remove "For backward compatibility".

@Tobion Tobion and 1 other commented on an outdated diff Jul 19, 2014

src/Symfony/Component/Routing/Loader/XmlFileLoader.php
+
+ /**
+ * Recursively parses the value of a "default" element.
+ *
+ * @param \DOMElement $node The node value
+ * @param string $path Full path of the XML file being processed
+ *
+ * @return array|bool|float|int|string The parsed value
+ *
+ * @throws \InvalidArgumentException when the XML is invalid
+ */
+ private function parseDefaultNode(\DOMElement $node, $path)
+ {
+ switch ($node->nodeName) {
+ case 'boolean':
+ return (bool) trim($node->nodeValue);
@Tobion

Tobion Jul 19, 2014

Member

This does not look like it works with 'false' correctly.

@xabbuh

xabbuh Jul 19, 2014

Member

You're right. Fixed it.

@Tobion Tobion commented on the diff Jul 19, 2014

...mponent/Routing/Loader/schema/routing/routing-1.0.xsd
@@ -26,7 +26,7 @@
<xsd:group name="configs">
<xsd:choice>
- <xsd:element name="default" nillable="true" type="element" />
+ <xsd:element name="default" nillable="true" type="default" />
<xsd:element name="requirement" type="element" />
@Tobion

Tobion Jul 19, 2014

Member

could you also add the required "key" attribute for "requirement" and "option" to the xsd? this does seem to be there yet. but xsd seems to be easy for you :)

@xabbuh

xabbuh Jul 19, 2014

Member

This is already be covered by the element definition in the XSD, isn't it?

@Tobion

Tobion Jul 19, 2014

Member

Oh true, didn't see that.

@Tobion Tobion and 1 other commented on an outdated diff Jul 19, 2014

...mponent/Routing/Loader/schema/routing/routing-1.0.xsd
@@ -26,7 +26,7 @@
<xsd:group name="configs">
<xsd:choice>
- <xsd:element name="default" nillable="true" type="element" />
+ <xsd:element name="default" nillable="true" type="default" />
<xsd:element name="requirement" type="element" />
<xsd:element name="option" type="element" />
<xsd:element name="condition" type="condition" />
@Tobion

Tobion Jul 19, 2014

Member

I don't see why condition has a special type. Just to ensure it only accepts strings? This would also apply for option and requirement.

@xabbuh

xabbuh Jul 19, 2014

Member

It doesn't have the key attribute. So it cannot be of type element. xsd:string should be sufficient though. What do you think?

@Tobion

Tobion Jul 19, 2014

Member

I think so too.

@xabbuh

xabbuh Jul 19, 2014

Member

Should it then be changed in a separate pull request to also address current Symfony versions?

@Tobion Tobion and 1 other commented on an outdated diff Jul 19, 2014

src/Symfony/Component/Routing/Loader/XmlFileLoader.php
+ private function parseDefaultNode(\DOMElement $node, $path)
+ {
+ switch ($node->nodeName) {
+ case 'boolean':
+ return (bool) trim($node->nodeValue);
+ case 'integer':
+ return (int) trim($node->nodeValue);
+ case 'float':
+ return (float) trim($node->nodeValue);
+ case 'string':
+ return trim($node->nodeValue);
+ case 'list':
+ $list = array();
+ $xpath = new \DOMXPath($node->ownerDocument);
+
+ foreach ($xpath->query('*', $node) as $element) {
@Tobion

Tobion Jul 19, 2014

Member

does this work with namespaces? what if an element from another namespace is inside? does the xsd allow that and what would happen here?

@xabbuh

xabbuh Jul 19, 2014

Member

It should. Though I don't think that the XPath is necessary at all. I changed that to use the childNodes attributes like it is done in the map part. The schema doesn't allow to add other elements. But one could just not specify the schema in their routing XML file. That could be a problem. I'll check that.

@xabbuh

xabbuh Jul 19, 2014

Member

The thing with additional element that are not allowed by the XSD is that they are rejected by the loader. That's because it throws exception if it detects elemens it does not know.

Member

xabbuh commented Jul 19, 2014

I just noticed that the new parseDefaultNode() method was capable to properly parse nodes with a namespace prefix. I fixed that and modified the already existing testLoadWithNamespacePrefix() to cover this too.

@Tobion Tobion and 1 other commented on an outdated diff Jul 19, 2014

src/Symfony/Component/Routing/Loader/XmlFileLoader.php
+
+ /**
+ * Recursively parses the value of a "default" element.
+ *
+ * @param \DOMElement $node The node value
+ * @param string $path Full path of the XML file being processed
+ *
+ * @return array|bool|float|int|string The parsed value
+ *
+ * @throws \InvalidArgumentException when the XML is invalid
+ */
+ private function parseDefaultNode(\DOMElement $node, $path)
+ {
+ switch ($node->localName) {
+ case 'boolean':
+ return 'true' === trim($node->nodeValue);
@Tobion

Tobion Jul 19, 2014

Member

xsd:boolean also allwos 0 and 1 AFAIK. So this won't work again.

@Tobion

Tobion Jul 19, 2014

Member

Also please add a test for all values.

@xabbuh

xabbuh Jul 19, 2014

Member

There should be a test for false. And as far as I know, you cannot use 1 and 0.

@Tobion

Tobion Jul 19, 2014

Member

I see you added a test for false 👍

@Tobion

Tobion Jul 19, 2014

Member

Note: Legal values for boolean are true, false, 1 (which indicates true), and 0 (which indicates false).
http://www.w3schools.com/schema/schema_dtypes_misc.asp

@xabbuh

xabbuh Jul 19, 2014

Member

I can check that again. But I'm pretty sure that the schemaValidateSource() method reported an error for 0 and 1. But I can of course add support for 0 and 1 though.

@xabbuh

xabbuh Jul 19, 2014

Member

Done. It works with 0 and 1.

@Tobion Tobion commented on the diff Jul 19, 2014

.../Component/Routing/Tests/Fixtures/scalar_defaults.xml
+
+ <route id="blog" path="/blog">
+ <default key="_controller">
+ <string>AcmeBlogBundle:Blog:index</string>
+ </default>
+ <default key="public">
+ <boolean>true</boolean>
+ </default>
+ <default key="page">
+ <integer>1</integer>
+ </default>
+ <default key="price">
+ <float>3.5</float>
+ </default>
+ <default key="archived">
+ <boolean>false</boolean>
Member

Tobion commented Jul 20, 2014

Can you please add a note about this in the routing changelog. Then I'm 👍 to merge this.

Member

xabbuh commented Jul 20, 2014

Of course, done.

Member

Tobion commented Jul 20, 2014

It's not there.

Member

xabbuh commented Jul 20, 2014

Sorry, I forgot to push.

Member

Tobion commented Jul 20, 2014

👍 now lets hear the other deciders

Member

xabbuh commented Jul 20, 2014

@Tobion Thanks for your support!

@fabpot fabpot and 1 other commented on an outdated diff Jul 25, 2014

src/Symfony/Component/Routing/CHANGELOG.md
@@ -1,6 +1,12 @@
CHANGELOG
=========
+2.6.0
+-----
+
+* Added support for `boolean`, `integer`, `float`, `string`, `list` and `map`
+ defaults.
@fabpot

fabpot Jul 25, 2014

Owner

You should be clear that this is only for the XML loader.

Owner

fabpot commented Jul 25, 2014

We already have support for different types in the service container... but the implementation is quite different. I think that being consistent between the two components is a good idea. The service container XML loader uses Symfony\Component\Config\Util\XmlUtils to parse complex data structure.

Member

Tobion commented Jul 25, 2014

@fabpot My opinion:

  1. the xml config parsing is magic (auto-convert types)
  2. AFAIK it does not all allow many things, like a string with null: 'null'.
  3. it does not support xsd validation because you cannot explicitly type things
  4. introducing the same in routing would be a BC break because defaults would be casted automatically now

I agree it would be nice to have the same logic everywhere, but I feel the better version is this one here. So I'd rather have the feature we implemented here inside the config component generalized.

Owner

fabpot commented Jul 25, 2014

@Tobion I understand your points. But let's be pragmatic here, we are talking about very rare cases. Nobody ever had this need. Using a default value as an array is a hack in the first place anyway. Using anything besides strings is also a hack (I know that null/booleans can have special meaning here.)

So I'm 👎.

Member

Tobion commented Jul 25, 2014

Then we need to validate defaults, so other loaders only allow plain strings as well. Like YAML or plain PHP.

Member

Tobion commented Jul 25, 2014

It's probably gonna be a bc break for many.

Member

xabbuh commented Jul 25, 2014

I'm not sure if nobody ever had the need because they didn't have the need or if it's just because they didn't know that the defaults could be used in such a way (we are just changing documentation towards adding notices on the usage of the defaults). Also, the current solution is rather inconsistent since you can do more things in the YAML and PHP loaders.

Contributor

sstok commented Jul 25, 2014

Using an array as default-value is used by Sylius for controller reusing, by using the route defaults as a configuration. So removing support would be a big BC break.

@fabpot fabpot added a commit that referenced this pull request Jul 27, 2014

@fabpot fabpot minor #11482 [Routing] simplify the XML schema file (xabbuh)
This PR was merged into the 2.4 branch.

Discussion
----------

[Routing] simplify the XML schema file

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

Conditions that are defined for certain routes are just simple strings.
Thus, there is no need to use a dedicated element when xsd:string does
the same (as mentioned by @tobion in #11394).

Commits
-------

dbac46a [Routing] simplify the XML schema file
9b5f56c
Contributor

sstok commented Aug 9, 2014

What is the status of this PR?

@stof stof commented on the diff Aug 12, 2014

src/Symfony/Component/Routing/Loader/XmlFileLoader.php
+ }
+
+ return $list;
+ case 'map':
+ $map = array();
+
+ foreach ($node->childNodes as $element) {
+ if (!$element instanceof \DOMElement) {
+ continue;
+ }
+
+ if (self::NAMESPACE_URI !== $element->namespaceURI) {
+ continue;
+ }
+
+ $map[$element->getAttribute('key')] = $this->parseDefaultNode($element, $path);
@stof

stof Aug 12, 2014

Member

you should check that the key attribute is set

@xabbuh

xabbuh Aug 12, 2014

Member

The key attribute for the map elements is mandatory.

@wouterj

wouterj Aug 12, 2014

Member

That's even a better reason to check it, isn't it?

@xabbuh

xabbuh Aug 12, 2014

Member

But we use the schema definition to validate the XML before.

@stof

stof Aug 13, 2014

Member

yeah, I forgot that the XSD already checked it

@stof stof and 1 other commented on an outdated diff Aug 12, 2014

...mponent/Routing/Loader/schema/routing/routing-1.0.xsd
@@ -55,6 +55,18 @@
<xsd:attribute name="methods" type="xsd:string" />
</xsd:complexType>
+ <xsd:complexType name="default" mixed="true">
+ <xsd:choice minOccurs="0" maxOccurs="unbounded">
@stof

stof Aug 12, 2014

Member

are you sure about maxOccurs="unbounded" ? You have a comment in the code saying that there can only be 1 element in it, which means maxOccurs="1"

@xabbuh

xabbuh Aug 12, 2014

Member

Good catch, thanks.

Member

stof commented Aug 12, 2014

does this syntax allow to put a null value inside a list or a map ?

Member

xabbuh commented Aug 12, 2014

@stof List and maps can now be null.

@Tobion Tobion and 1 other commented on an outdated diff Aug 13, 2014

src/Symfony/Component/Routing/Loader/XmlFileLoader.php
@@ -242,4 +241,101 @@ private function parseConfigs(\DOMElement $node, $path)
return array($defaults, $requirements, $options, $condition);
}
+
+ /**
+ * Parses the "default" elements.
+ *
+ * @param \DOMElement $element The "default" element to parse
+ * @param string $path Full path of the XML file being processed
+ *
+ * @return array|bool|float|int|string|null The parsed value of the "default" element
+ */
+ private function parseDefaultsConfig(\DOMElement $element, $path)
+ {
+ if ($element->hasAttribute('xsi:nil') && 'true' == $element->getAttribute('xsi:nil')) {
@Tobion

Tobion Aug 13, 2014

Member

Actually I think we should use http://php.net/manual/en/domelement.getattributens.php to retrieve the attribute in the namespace.

@xabbuh

xabbuh Aug 14, 2014

Member

Good idea. Changed it here and in line 292.

@Tobion Tobion and 1 other commented on an outdated diff Aug 14, 2014

src/Symfony/Component/Routing/Loader/XmlFileLoader.php
@@ -242,4 +241,101 @@ private function parseConfigs(\DOMElement $node, $path)
return array($defaults, $requirements, $options, $condition);
}
+
+ /**
+ * Parses the "default" elements.
+ *
+ * @param \DOMElement $element The "default" element to parse
+ * @param string $path Full path of the XML file being processed
+ *
+ * @return array|bool|float|int|string|null The parsed value of the "default" element
+ */
+ private function parseDefaultsConfig(\DOMElement $element, $path)
+ {
+ if ($element->hasAttributeNS('http://www.w3.org/2001/XMLSchema-instance', 'nil') && 'true' == $element->getAttributeNS('http://www.w3.org/2001/XMLSchema-instance', 'nil')) {
@Tobion

Tobion Aug 14, 2014

Member

what happens when 1 or 0 is used for xsi:nil as a boolean http://www.w3.org/TR/2004/REC-xmlschema-2-20041028/datatypes.html#boolean

@xabbuh

xabbuh Aug 14, 2014

Member

Thanks, you're right. This didn't work in 2.3 before. I submitted a fix for this (see #11672).

@xabbuh xabbuh added a commit to xabbuh/symfony that referenced this pull request Aug 14, 2014

@xabbuh xabbuh fix handling of nullable XML attributes
As @Tobion pointed out in #11394, true and 1 are valid values in
boolean XML attributes. The XmlFileLoader didn't handle 1 values
properly.
6deca28

@xabbuh xabbuh added a commit to xabbuh/symfony that referenced this pull request Aug 15, 2014

@xabbuh xabbuh fix handling of nullable XML attributes
As @Tobion pointed out in #11394, true and 1 are valid values in
boolean XML attributes. The XmlFileLoader didn't handle 1 values
properly.
29e452d

@xabbuh xabbuh added a commit to xabbuh/symfony that referenced this pull request Aug 15, 2014

@xabbuh xabbuh fix handling of nullable XML attributes
As @Tobion pointed out in #11394, true and 1 are valid values in
boolean XML attributes. The XmlFileLoader didn't handle 1 values
properly.
55eeb89

@xabbuh xabbuh added a commit to xabbuh/symfony that referenced this pull request Aug 15, 2014

@xabbuh xabbuh fix handling of nullable XML attributes
As @Tobion pointed out in #11394, true and 1 are valid values in
boolean XML attributes. The XmlFileLoader didn't handle 1 values
properly.
7b4d4b6

@Tobion Tobion added a commit that referenced this pull request Aug 20, 2014

@Tobion Tobion bug #11672 [Routing] fix handling of nullable XML attributes (xabbuh)
This PR was merged into the 2.3 branch.

Discussion
----------

[Routing] fix handling of nullable XML attributes

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

As @Tobion pointed out in #11394, ``true`` and ``1`` are valid values in boolean XML attributes. The XmlFileLoader didn't handle ``1`` values properly.

Commits
-------

7b4d4b6 fix handling of nullable XML attributes
f262b01
Member

Tobion commented Oct 6, 2014

@fabpot considering all other loaders except xml already "supported" array defaults, we have two ways to deal with this

  1. "remove" array default support in the other loaders, which will probably be a bc break for some
  2. add support for array defaults in xml to be consistent and merge this PR

I'd vote for 2. because its not a bc break and it should not be a problem in php to support arrays. we deprecated apache dumper where it would probably be tedious to support arrays. so thats not a problem. also the xml loader already explicitly supported null, so also supporting other scalars like int makes sense. fabien, what do you prefer?

Owner

fabpot commented Oct 7, 2014

The support for non-string values in other formats was not a conscious decision; it just happens to work because there is no restrictions in place. So, I'm still against this change.

Member

Tobion commented Oct 7, 2014

Well actually it was, at least for null: #7635

Member

Tobion commented Oct 8, 2014

And according to RequestContext::setParameter a parameter can be mixed.

Owner

fabpot commented Jun 22, 2016

Re-reading this old PR, I still thing that defaults should only be strings or null. When parsing a path, you can only get back strings anyway, so allowing defaults to be something else seems wrong to me. I'm in favor of deprecating the possibility to use any other kind of value.

Member

Tobion commented Jun 22, 2016

One point of routing is that you can change URIs without chaning the code to parse/generate them. Since array params can be used for query strings, at one point it would also make sense to allow arrays in the path. For example, URI templates standard supports that by expanding arrays to comma-separated values.
So removing non-string defaults would be a step in the wrong direction IMO.

Member

Tobion commented Jun 22, 2016

Also with the introduction of scalar type hints, what if a controller parameter is typehinted to int for example? I guess this will be cast implicitly at the moment. But with strict types, it would cause an error. So in theory either the router itself could cast params (for example based on the requirement) or the code that calls the controller. So then route params would need to be non-strings as well.

Owner

fabpot commented Jun 22, 2016

ok, you're right. Let's finish this one then.

Member

stof commented Jun 22, 2016

@Tobion strict types are irrelevant here, because the strict mode for arguments is determined by the calling code, not by the file declaring the controller, and the Kernel class (which is calling the controller) is not in strict mode

Member

Tobion commented Jun 22, 2016

@stof I know. But who says Symfony 4 will not add scalar type hints in certain places and maybe strict types?

@xabbuh xabbuh [Routing] data type support for defaults
As pointed out in symfony/symfony-docs#4017, the XmlFileLoader was not
capable of defining array default values. Additionally, this commit
adds support for handling associative arrays, boolean, integer, float
and string data types.
120b35c
Member

xabbuh commented Jun 23, 2016

rebased here, if I don't miss anything all comments made so far have already been addressed

Owner

fabpot commented Jun 23, 2016

Thank you @xabbuh.

@fabpot fabpot merged commit 120b35c into symfony:master Jun 23, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

@fabpot fabpot added a commit that referenced this pull request Jun 23, 2016

@fabpot fabpot feature #11394 [Routing] support for array values in route defaults (…
…xabbuh)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Routing] support for array values in route defaults

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

As pointed out in symfony/symfony-docs#4017, the ``XmlFileLoader`` was not capable of defining array default values.

- [x] array values
- [x] integer values
- [x] float values
- [x] boolean

Commits
-------

120b35c [Routing] data type support for defaults
a80b45d

@xabbuh xabbuh deleted the xabbuh:route-collection-defaults-xml branch Jun 23, 2016

Member

Tobion commented Jun 23, 2016

Maybe we should rename boolean and integer elements to bool and int elements as the short names are now more standard considering scalar typehints etc.

@fabpot fabpot added a commit that referenced this pull request Jul 1, 2016

@fabpot fabpot minor #19157 [Routing] rename boolean and integer to bool and int (xa…
…bbuh)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Routing] rename boolean and integer to bool and int

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #11394 (comment)
| License       | MIT
| Doc PR        |

This addresses @Tobion's comment in #11394 (comment).

@symfony/deciders What do you think? Should we change the element names to how we use the types in PHP code or should we stick with XML element names that rather match the types as defined by the XML schema?

Commits
-------

808dcc8 rename boolean and integer to bool and int
35f201f

@xabbuh xabbuh added a commit to xabbuh/symfony that referenced this pull request May 23, 2017

@xabbuh xabbuh [Routing] remove an unused routing fixture
This was initially removed in #13361 and accidentally added again
in #11394.
6f67221

@fabpot fabpot added a commit that referenced this pull request May 24, 2017

@fabpot fabpot minor #22881 [Routing] remove an unused routing fixture (xabbuh)
This PR was merged into the 3.2 branch.

Discussion
----------

[Routing] remove an unused routing fixture

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

This was initially removed in #13361 and accidentally added again
in #11394.

Commits
-------

6f67221 [Routing] remove an unused routing fixture
df443f0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment