Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
9.7 KB

This converts valid_number_step() to a component (Number::validStep()), and moves the unit test to phpunit, with dataproviders.

Status: Needs review » Needs work

The last submitted patch, system-validnumber-phpunit-2030173-01.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, system-validnumber-phpunit-2030173-01.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
12.1 KB

Forgot to add the new Number component file.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

woo, looks good thanks:)

YesCT’s picture

Issue tags: +PHPUnit, +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

YesCT’s picture

FileSize
334.7 KB

This increases phpunit test code coverage from

patch Lines Functions and Methods Classes and Traits
no patch 18.22% 2742 / 15050 19.93% 368 / 1846 19.32% 34 / 176
patch 18.25% 2748 / 15056 19.98% 369 / 1847 19.77% 35 / 177

codecoveragewithpatch.png

catch’s picture

Issue tags: -PHPUnit, -RTBC July 1

Status: Reviewed & tested by the community » Needs work
Issue tags: +PHPUnit, +RTBC July 1

The last submitted patch, system-validnumber-phpunit-2030173-05.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
12.09 KB

Re-rolled #5.

dawehner’s picture

That is a really nice test!

+++ b/core/lib/Drupal/Component/Utility/Number.phpundefined
@@ -0,0 +1,60 @@
+    // The fractional part of a double has 53 bits. The greatest number that could
...
+    // $step * 2^53, then dividing by $step will result in a very small remainder.
...
+    // fractional part of a float has 24 bits. That means remainders smaller than

Some lines exceed the 80 chars, so lets fix them.

+++ b/core/tests/Drupal/Tests/Component/Utility/NumberTest.phpundefined
@@ -0,0 +1,125 @@
+class NumberTest extends UnitTestCase {
+  public static function getInfo() {

Lets add an empty line here.

jhedstrom’s picture

Should address #12.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/common.incundefined
@@ -628,47 +629,13 @@ function valid_url($url, $absolute = FALSE) {
 /**
  * Verifies that a number is a multiple of a given step.
  *
- * The implementation assumes it is dealing with IEEE 754 double precision
- * floating point numbers that are used by PHP on most systems.
+ * @see \Drupal\Component\Utility\Number::validStep()
  *
- * This is based on the number/range verification methods of webkit.
- *
- * @param $value
- *   The value that needs to be checked.
- * @param $step
- *   The step scale factor. Must be positive.
- * @param $offset
- *   (optional) An offset, to which the difference must be a multiple of the
- *   given step.
- *
- * @return bool
- *   TRUE if no step mismatch has occured, or FALSE otherwise.
- *
- * @see http://opensource.apple.com/source/WebCore/WebCore-1298/html/NumberInputType.cpp
+ * @deprecated as of Drupal 8.0. Use
+ *   \Drupal\Component\Utility\Number::validStep() directly instead
  */
 function valid_number_step($value, $step, $offset = 0.0) {

I'm not sure that we should be removing the documentation of the parameters and return value here... we can remove the the details and replace with the @see...

So I think this should look like this...

+++ b/core/includes/common.incundefined
@@ -628,47 +629,13 @@ function valid_url($url, $absolute = FALSE) {
 /**
  * Verifies that a number is a multiple of a given step.
  *
  * @see \Drupal\Component\Utility\Number::validStep()
  *
  * @param $value
  *   The value that needs to be checked.
  * @param $step
  *   The step scale factor. Must be positive.
  * @param $offset
  *   (optional) An offset, to which the difference must be a multiple of the
  *   given step.
  *
  * @return bool
  *   TRUE if no step mismatch has occured, or FALSE otherwise.
  *
  * @deprecated as of Drupal 8.0. Use
  *   \Drupal\Component\Utility\Number::validStep() directly instead
  */
 function valid_number_step($value, $step, $offset = 0.0) {

... I know I committed a patch by @jhedstrom that removed docs like this is the past but I now consider that a mistake...

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
12.17 KB
751 bytes

This adds those params back. The reason I'd been removing them was due to some really large docblocks (not including the params/return), that seemed silly to duplicate on both the deprecated function and the new component method.

ParisLiakos’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

oh everyone forgot this issue i guess:P

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
12.17 KB

Re-roll of #16.

ParisLiakos’s picture

Issue tags: -Needs reroll
+++ b/core/tests/Drupal/Tests/Component/Utility/NumberTest.php
@@ -0,0 +1,126 @@
+    $this->assertEquals($return, $expected);
...
+    $this->assertEquals($return, $expected);

$expected should be the first argument

jhedstrom’s picture

Flipped the argument order as per #19.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

ty

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

jhedstrom’s picture