Skip to content

[Form] Removed Integer scale option #9542

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

Closed
wants to merge 3 commits into from
Closed

[Form] Removed Integer scale option #9542

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6907be
[Form] Removed Integer scale option
dlabouesse Apr 3, 2018
fd9012e
Removed scale.rst.inc because it was included into number.rst only
dlabouesse Apr 5, 2018
1f820e4
Moved scale option into the overriden section of IntegerType
dlabouesse Apr 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions reference/forms/types/integer.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ integers. By default, all non-integer values (e.g. 6.78) will round down
| Rendered as | ``input`` ``number`` field |
+-------------+-----------------------------------------------------------------------+
| Options | - `grouping`_ |
| | - `scale`_ |
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we should better move it to the Overridden section, saying that the reverse transformation cast the value to an integer, ignoring the scale option at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @HeahDude! Sorry for my late reply.
I'm not really convinced by the interest of keeping this point in the documentation if the user can not customize its behavior by any way. As any value that might have been set by the user to scale of IntegerType will be overridden by 0, it make sense for me to remove this section from its documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand, but this option is defined anyway and the type can still be extended to customize the transformation process. I would prefer to document the option and its overridden behavior instead of not mentioning anything.

| | - `rounding_mode`_ |
+-------------+-----------------------------------------------------------------------+
| Overridden | - `compound`_ |
Expand Down Expand Up @@ -47,8 +46,6 @@ Field Options

.. include:: /reference/forms/types/options/grouping.rst.inc

.. include:: /reference/forms/types/options/scale.rst.inc

rounding_mode
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that the rounding_mode option is not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, rounding_mode option is also hardcoded to ROUND_DOWN for InterType. (https://github.com/symfony/symfony/blob/a90cd13fa412a082bee8eda1bab2a5bcc8f18cb7/src/Symfony/Component/Form/Extension/Core/DataTransformer/IntegerToLocalizedStringTransformer.php#L32)

Copy link
Member

Choose a reason for hiding this comment

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

That's only the default value. But inside the form type's buildForm() method the option is passed to the transformer and it will be used then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops! Yes you're right. Sorry for my mistake. I've updated the changes accordingly.
BTW, scale.rst.inc file is now only included in number.rst file, so shall we copy it's content directly to into number.rst to remove the scale.rst.inc file?

Copy link
Member

Choose a reason for hiding this comment

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

Moving the text makes sense to me.

~~~~~~~~~~~~~

Expand Down
14 changes: 13 additions & 1 deletion reference/forms/types/number.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,19 @@ Field Options

.. include:: /reference/forms/types/options/grouping.rst.inc

.. include:: /reference/forms/types/options/scale.rst.inc
scale
~~~~~

.. versionadded:: 2.7
The ``scale`` option was introduced in Symfony 2.7. Prior to Symfony 2.7,
it was known as ``precision``.

**type**: ``integer`` **default**: Locale-specific (usually around ``3``)

This specifies how many decimals will be allowed until the field rounds
the submitted value (via ``rounding_mode``). For example, if ``scale`` is set
to ``2``, a submitted value of ``20.123`` will be rounded to, for example,
``20.12`` (depending on your `rounding_mode`_).

rounding_mode
~~~~~~~~~~~~~
Expand Down
13 changes: 0 additions & 13 deletions reference/forms/types/options/scale.rst.inc
View file
Open in desktop

This file was deleted.