8000 [Form][BC break] Decimal fields cannot be type-hinted as float · Issue #32124 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form][BC break] Decimal fields cannot be type-hinted as float #32124

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
Closed
yceruto opened this issue Jun 20, 2019 · 3 comments
Closed

[Form][BC break] Decimal fields cannot be type-hinted as float #32124

yceruto opened this issue Jun 20, 2019 · 3 comments
Labels
Bug Form Status: Needs Review

Comments

@yceruto
Copy link
Member
yceruto commented Jun 20, 2019 •
edited
Loading

Symfony version(s) affected: 4.3

Description
I have the follow entity and field definition:

class Product
{
    /**
     * @ORM\Column(type="decimal", precision=10, scale=2)
     */
    private $price = 0;

    public function getPrice(): float
    {
        return $this->price;
    }

    public function setPrice(float $price)
    {
        $this->price = $price;
    }
}

and a form field whose "type" is being guessed by DoctrineOrmTypeGuesser:

$builder->add('price');

this works well before 4.3, but after upgrade this app, when the form is being built, this error appears:

Unable to transform data for property path "price": Expected a numeric string.

Possible Solution
It will depend on the right way of type-hinting decimal fields.

Even if #30893 helps to solve a real problem with Doctrine's change detection, I think we must fix the BC break, either relaxing the transformation constraint allowing numeric values:

symfony/src/Symfony/Component/Form/Extension/Core/DataTransformer/StringToFloatTransformer.php

Lines 37 to 39 in 030396a

if (!\is_string($value) || !is_numeric($value)) {
throw new TransformationFailedException('Expected a numeric string.');
} 

-if (!\is_string($value) || !is_numeric($value)) {
-    throw new TransformationFailedException('Expected a numeric string.');
+if (!\is_numeric($value)) {
+    throw new TransformationFailedException('Expected a number or a numeric string.');
}

or reverting the change in DoctrineOrmTypeGuesser:

symfony/src/Symfony/Bridge/Doctrine/Form/DoctrineOrmTypeGuesser.php

Lines 77 to 78 in 030396a

case Type::DECIMAL:
return new TypeGuess('Symfony\Component\Form\Extension\Core\Type\NumberType', ['input' => 'string'], Guess::MEDIUM_CONFIDENCE);

deprecating this default, for later change in 5.0 to input = string.

WDYT?
@xabbuh
Copy link
Member
xabbuh commented Jun 20, 2019

This looks like the same as #31905. While I still think that using float as the type here is a mistake I think it does not hurt if we relax the StringToFloatTransformer (see #32125).

@xabbuh xabbuh mentioned this issue Jun 20, 2019
Merged
@yceruto
Copy link
Member Author
yceruto commented Jun 20, 2019 •
edited
Loading

Is this the right way to work around the Doctrine's change detection issue in the model side? '8.000' -> '8.000':

class Product
{
    /**
     * @ORM\Column(type="decimal", precision=10, scale=3)
     */
    private $price = '0';

    public function getPrice(): string
    {
        return $this->price;
    }

    public function setPrice(string $price)
    {
        $this->price = $price;
    }
}

If we use float would incur in the same problem '8.000' -> 8,
but then it looks like a wrong type from the design point of view.

@xabbuh
Copy link
Member
xabbuh commented Jun 20, 2019

@yceruto Yes, that looks good. If you debug your former code, you should be able to see that the $price property is a string and not a float when the object is populated by Doctrine based on the data stored in the database.

@fabpot fabpot closed this as completed Jun 22, 2019
fabpot added a commit that referenced this issue Jun 22, 2019
@fabpot
bug #32125 [Form] accept floats for input="string" in NumberType (xab…
0dbf477 
…buh)

This PR was merged into the 4.3 branch.

Discussion
----------

[Form] accept floats for input="string" in NumberType

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31905, #32124
| License       | MIT
| Doc PR        |

Commits
-------

2abf855 accept floats for input="string" in NumberType
@spackmat spackmat mentioned this issue Jun 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Form Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@fabpot @xabbuh @yceruto @carsonbot
0