[Yaml] deprecate comma separators in floats #18785
Conversation
xabbuh
commented
May 14, 2016
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | #18178 |
| License | MIT |
| Doc PR |
| @@ -591,6 +591,7 @@ private static function evaluateScalar($scalar, $flags, $references = array()) | |||
| case 0 === strpos($scalar, '!!binary '): | |||
| return self::evaluateBinaryScalar(substr($scalar, 9)); | |||
| case preg_match('/^(-|\+)?[0-9][0-9,]*(\.[0-9_]+)?$/', $scalar): | |||
| @trigger_error('Using the comma as a group separator for floats is deprecated since version 3.2 and will be removed in 4.0.', E_USER_DEPRECATED); | |||
There was a problem hiding this comment.
If I understand this regexp right, the deprecation error will be triggered for example for numbers like 123.45 but it shouldn't. We'd need to test explicitly the presence of the comma.
There was a problem hiding this comment.
yeah, the order of regexps should be changed, to match the standard regex first to handle the case of values matching the spec
This will require to duplicate the line return (float) str_replace(array(',', '_'), '', $scalar); instead of using a fallthrough, but this is not an issue.
There was a problem hiding this comment.
These cases are already handled further up where we have the is_numeric() check (and we have tests covering that this is working).
There was a problem hiding this comment.
As @javiereguiluz said, this deprecation will be triggered with 123.45, which is not correct. IMO, the * should be replaced with a + to ensure that a , is indeed used.
There was a problem hiding this comment.
actually, the + won't be enough ; we need to test that [0-9]+,[0-9]*(?:\.[0-9_]+)? matches this I think (but I'm not too sure too)
There was a problem hiding this comment.
But it should be triggered for 123.45_67 though (I think), as there are no cases for that above this case, and that is valid though. And I think a _ may have been forgotten in the part before the . actually.
IMO, we should do a strpos test to test that a , is indeed in the match, and then only trigger the deprecation
There was a problem hiding this comment.
@Taluu You are right. I opened #18858 which should fix this issue.
And I think a
_may have been forgotten in the part before the.actually.
I think it's okay the way it is. We only introduced the _ for 3.2 and I think people should not start mixing _ and , then. If they want to use _, they should just move away from , as well.
|
👍 Status: reviewed |
|
Thank you @xabbuh. |
This PR was merged into the 3.2-dev branch. Discussion ---------- [Yaml] deprecate comma separators in floats | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #18178 | License | MIT | Doc PR | Commits ------- 3f3623d [Yaml] deprecate comma separators in floats