silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

NumericField setScale() does not update decimals #9463

Open Taitava opened 4 years ago

Taitava commented 4 years ago

Affected Version 4.5.1

Description

When creating a new NumericField in a custom Form and setting a float value to the field, the decimals will get stripped off even if the NumericField::setScale() method is called right after creating the field (before rendering it). This is because the field's construction process casts the value as an integer.

Steps to Reproduce

<?php

use SilverStripe\Control\Controller;
use SilverStripe\Forms\FieldList;
use SilverStripe\Forms\Form;
use SilverStripe\Forms\NumericField;

class SomeController extends Controller
{
    function SomeForm()
    {
        $fields = new FieldList(
            $numeric_field = new NumericField('SomeField', 'Some field', 1.5) // The value 1.5 will be casted to an integer 1
        );
        $numeric_field->setScale(2);
        return new Form($this, 'SomeForm', $fields);
    }
}

The scale is 0 during the construction. The field constructor calls NumericField::setValue(), which performs casting: https://github.com/silverstripe/silverstripe-framework/blob/3ad4b93daacf3dc3d76a3da6c0287827604a631a/src/Forms/NumericField.php#L142-L147 (Note $this->originalValue, I'll come to that soon.)

NumericField::setScale() makes the field float/integer, but does not correct the decimals of the current value: https://github.com/silverstripe/silverstripe-framework/blob/3ad4b93daacf3dc3d76a3da6c0287827604a631a/src/Forms/NumericField.php#L307-L318

So I suggest that we:

A secondary solution would be to add a $scale parameter to NumericField::__construct() and set the scale before setting the value. Or maybe do both of these?

Taitava commented 3 years ago

I can provide a PR for this. I'd just like to confirm, are the two solutions mentioned at the end of my earlier comment okay? In addition to actually fix the setScale() method, I'd like to add the $scale parameter to the __construct() method.