Skip to content
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

Jump to bottom

[Form] IntegerType: reject submitted non-integer numbers #30329

Merged
merged 1 commit into from Feb 23, 2019
Merged

[Form] IntegerType: reject submitted non-integer numbers #30329

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions ...ony/Component/Form/Extension/Core/DataTransformer/IntegerToLocalizedStringTransformer.php
View file
Open in desktop
Expand Up @@ -11,6 +11,8 @@

namespace Symfony\Component\Form\Extension\Core\DataTransformer;

use Symfony\Component\Form\Exception\TransformationFailedException;

/**
* Transforms between an integer and a localized number with grouping
* (each thousand) and comma separators.
Expand Down Expand Up @@ -40,6 +42,12 @@ public function __construct($scale = 0, $grouping = false, $roundingMode = self:
*/
public function reverseTransform($value)
{
$decimalSeparator = $this->getNumberFormatter()->getSymbol(\NumberFormatter::DECIMAL_SEPARATOR_SYMBOL);

if (\is_string($value) && false !== strpos($value, $decimalSeparator)) {
Copy link
Member

Choose a reason for hiding this comment

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

The transformation is invoked in reverse order in DateIntervalType when widget = integer, also in the model layer, so it could receive a real floating value here, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you give an example? I am not sure that I understand what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

symfony/src/Symfony/Component/Form/Extension/Core/Type/DateIntervalType.php

Lines 116 to 122 in 5a3e894

if ('integer' === $options['widget']) {
$childForm->addModelTransformer(
new ReversedTransformer(
new IntegerToLocalizedStringTransformer()
)
);
}

Copy link
Member

Choose a reason for hiding this comment

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

Unlike IntegerType this transformation is invoked for model layer in reversed order, so I guess we should deal with real float values here.

Copy link
Member

Choose a reason for hiding this comment

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

Basically, this is what I suggest:

if (false !== strpos((string) $value, $decimalSeparator)) {

Copy link
Member

Choose a reason for hiding this comment

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

Although, technically that should never happen because DateIntervalToArrayTransformer, so ignore my suggestion.

throw new TransformationFailedException(sprintf('The value "%s" is not a valid integer.', $value));
}

$result = parent::reverseTransform($value);

return null !== $result ? (int) $result : null;
Expand Down
35 changes: 28 additions & 7 deletions ...ent/Form/Tests/Extension/Core/DataTransformer/IntegerToLocalizedStringTransformerTest.php
View file
Open in desktop
Expand Up @@ -95,9 +95,7 @@ public function testReverseTransform()
$transformer = new IntegerToLocalizedStringTransformer();

$this->assertEquals(1, $transformer->reverseTransform('1'));
$this->assertEquals(1, $transformer->reverseTransform('1,5'));
$this->assertEquals(1234, $transformer->reverseTransform('1234,5'));
$this->assertEquals(12345, $transformer->reverseTransform('12345,912'));
$this->assertEquals(12345, $transformer->reverseTransform('12345'));
}

public function testReverseTransformEmpty()
Expand All @@ -116,10 +114,10 @@ public function testReverseTransformWithGrouping()

$transformer = new IntegerToLocalizedStringTransformer(null, true);

$this->assertEquals(1234, $transformer->reverseTransform('1.234,5'));
$this->assertEquals(12345, $transformer->reverseTransform('12.345,912'));
$this->assertEquals(1234, $transformer->reverseTransform('1234,5'));
$this->assertEquals(12345, $transformer->reverseTransform('12345,912'));
$this->assertEquals(1234, $transformer->reverseTransform('1.234'));
$this->assertEquals(12345, $transformer->reverseTransform('12.345'));
$this->assertEquals(1234, $transformer->reverseTransform('1234'));
$this->assertEquals(12345, $transformer->reverseTransform('12345'));
}

public function reverseTransformWithRoundingProvider()
Expand Down Expand Up @@ -203,6 +201,29 @@ public function testReverseTransformExpectsValidNumber()
$transformer->reverseTransform('foo');
}

/**
* @dataProvider floatNumberProvider
* @expectedException \Symfony\Component\Form\Exception\TransformationFailedException
*/
public function testReverseTransformExpectsInteger($number, $locale)
{
IntlTestHelper::requireFullIntl($this, false);

\Locale::setDefault($locale);

$transformer = new IntegerToLocalizedStringTransformer();

$transformer->reverseTransform($number);
}

public function floatNumberProvider()
{
return [
['12345.912', 'en'],
['1.234,5', 'de_DE'],
];
}

/**
* @expectedException \Symfony\Component\Form\Exception\TransformationFailedException
*/
Expand Down
7 changes: 4 additions & 3 deletions src/Symfony/Component/Form/Tests/Extension/Core/Type/IntegerTypeTest.php
View file
Open in desktop
Expand Up @@ -24,14 +24,15 @@ protected function setUp()
parent::setUp();
}

public function testSubmitCastsToInteger()
public function testSubmitRejectsFloats()
{
$form = $this->factory->create(static::TESTED_TYPE);

$form->submit('1.678');

$this->assertSame(1, $form->getData());
$this->assertSame('1', $form->getViewData());
$this->assertTrue($form->isSubmitted());
$this->assertFalse($form->isValid());
$this->assertFalse($form->isSynchronized());
}

public function testSubmitNull($expected = null, $norm = null, $view = null)
Expand Down