Skip to content

[Routing] fix handling of nullable XML attributes #11672

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.

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
Aug 20, 2014

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Aug 14, 2014

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.

@xabbuh xabbuh changed the title fix handling of nullable XML attributes [Routing] fix handling of nullable XML attributes Aug 14, 2014
$routeCollection = $loader->load('null_values.xml');
$route = $routeCollection->get('blog_show');

$this->assertNull($route->getDefault('foo'));
Copy link
Contributor

Choose a reason for hiding this comment

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

this alone doesnt test anything since getDefault also return null when it does not exist. so u need to also use hasDefault

@xabbuh
Copy link
Member Author

xabbuh commented Aug 15, 2014

@Tobion I didn't add any tests for false or 0 values yet. Your comment is now just hidden because of the remove root element attributes. Do you still want me to add tests for them respectively?

@Tobion
Copy link
Contributor

Tobion commented Aug 15, 2014

Why not. It's easy and makes sure the implementation does not only check hasAttribute but also the value. Just add it in the same null_values.xml file.

As @Tobion pointed out in symfony#11394, true and 1 are valid values in
boolean XML attributes. The XmlFileLoader didn't handle 1 values
properly.
@xabbuh
Copy link
Member Author

xabbuh commented Aug 15, 2014

Okay, added them.

@Tobion
Copy link
Contributor

Tobion commented Aug 15, 2014

👍

2 similar comments
@nicolas-grekas
Copy link
Member

👍

@stof
Copy link
Member

stof commented Aug 18, 2014

👍

@stof stof added the Routing label Aug 18, 2014
@Tobion
Copy link
Contributor

Tobion commented Aug 20, 2014

Thanks for fixing this bug @xabbuh.

@Tobion Tobion merged commit 7b4d4b6 into symfony:2.3 Aug 20, 2014
Tobion added a commit that referenced this pull request Aug 20, 2014
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
@Tobion
Copy link
Contributor

Tobion commented Aug 20, 2014

My first merge :)

@xabbuh xabbuh deleted the routing-xml-null-values branch August 20, 2014 09:14
@xabbuh
Copy link
Member Author

xabbuh commented Aug 20, 2014

@Tobion Thanks for all your feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants