Use strict comparison for onlyExpectedArguments #121

wage-slave-montyakku · 2017-01-30T12:37:39Z

以下の2箇所は == ではなく === で比較しないと正しく無いのにテストをすり抜けるパターンが出てしまう

下記は修正されているので、恐らくバグだろう

https://github.com/Codeception/AspectMock/blob/cb0f376/src/AspectMock/Proxy/Verifier.php#L68

phpは0と文字列の比較は、==だとtrueになってしまう

var_dump(0=="文字列");  //bool(true)

例えば引数の配列に0が入っていて、比較対象を文字列にした場合問題が起きる

$args = [1,2, "文字列"] ;
$params = [1,2, 0] 
var_dump($this->onlyExpectedArguments($params, $args) == $params);  //bool(true)

// 同じじゃねーだろ！！

Nitpick-CI · 2017-01-30T12:37:44Z

src/AspectMock/Proxy/Verifier.php

@@ -114,7 +114,7 @@ public function verifyInvokedMultipleTimes($name, $times, $params = null)
        if (is_array($params)) {
            $equals = 0;
            foreach ($calls as $args) {
-                if ($this->onlyExpectedArguments($params, $args) == $params) $equals++;
+                if ($this->onlyExpectedArguments($params, $args) === $params) $equals++;


Inline control structures are not allowed

それもとからやん!!

Nitpick-CI · 2017-01-30T12:37:44Z

src/AspectMock/Proxy/Verifier.php

@@ -153,7 +153,7 @@ public function verifyNeverInvoked($name, $params = null)
             if (empty($calls)) return;
             $params = ArgumentsFormatter::toString($params);
             foreach ($calls as $args) {
-                 if ($this->onlyExpectedArguments($params, $args) == $params) throw new fail(sprintf($this->neverInvoked, $this->className));
+                 if ($this->onlyExpectedArguments($params, $args) === $params) throw new fail(sprintf($this->neverInvoked, $this->className));


Inline control structures are not allowed

それもとからやん!!

Nitpick-CI · 2017-01-30T12:45:26Z

src/AspectMock/Proxy/Verifier.php

@@ -153,7 +155,9 @@ public function verifyNeverInvoked($name, $params = null)
             if (empty($calls)) return;
             $params = ArgumentsFormatter::toString($params);
             foreach ($calls as $args) {
-                 if ($this->onlyExpectedArguments($params, $args) == $params) throw new fail(sprintf($this->neverInvoked, $this->className));
+                 if ($this->onlyExpectedArguments($params, $args) === $params) {


Line indented incorrectly; expected 16 spaces, found 17

Nitpick-CI · 2017-01-30T12:45:27Z

src/AspectMock/Proxy/Verifier.php

-                 if ($this->onlyExpectedArguments($params, $args) == $params) throw new fail(sprintf($this->neverInvoked, $this->className));
+                 if ($this->onlyExpectedArguments($params, $args) === $params) {
+                     throw new fail(sprintf($this->neverInvoked, $this->className));
+                 }


Line indented incorrectly; expected 20 spaces, found 17

これも　元からやろ!!

Nitpick-CI · 2017-01-30T12:56:49Z

src/AspectMock/Proxy/Verifier.php

-         }
-         if (count($calls)) throw new fail(sprintf($this->neverInvoked, $this->className.$separator.$name));        
+        if (is_array($params)) {
+            if (empty($calls)) return;


Inline control structures are not allowed

if (empty($calls)) { return; }

Nitpick-CI · 2017-01-30T12:56:49Z

src/AspectMock/Proxy/Verifier.php

+            }
+            return;
+        }
+        if (count($calls)) throw new fail(sprintf($this->neverInvoked, $this->className.$separator.$name));


Inline control structures are not allowed

sergeyklay · 2017-01-30T13:20:06Z

src/AspectMock/Proxy/Verifier.php

-         }
-         if (count($calls)) throw new fail(sprintf($this->neverInvoked, $this->className.$separator.$name));        
+        if (is_array($params)) {
+            if (empty($calls)) return;


if (empty($calls)) { return; }

wage-slave-montyakku · 2017-01-31T03:14:17Z

指摘事項は修正済みです

sergeyklay · 2017-01-31T06:24:33Z

I will keep this open for a day or two to allow other contributors a chance to speak up, and to take a fresh look at it later. But at the moment it seems good to me.

DavertMik · 2017-01-31T19:43:08Z

Thanks for the PR and for fixing the codestyle.
I'd like to ask you write next comments in English, so we could understand them. Thanks

wage-slave-montyakku · 2017-02-01T12:30:57Z

Thank you!! 😄

入力値チェックの比較を == から === に変更

c526d8d

Nitpick-CI reviewed Jan 30, 2017

View reviewed changes

Inline control structures are not allowed と言われたのでワンライン辞めた

ddd01ce

Nitpick-CI reviewed Jan 30, 2017

View reviewed changes

Line indented incorrectly; と言われた部分を修正

039ebbe

Nitpick-CI reviewed Jan 30, 2017

View reviewed changes

Inline control structures are not allowed を修正

43b9452

sergeyklay suggested changes Jan 30, 2017

View reviewed changes

sergeyklay approved these changes Jan 31, 2017

View reviewed changes

DavertMik merged commit 7a13a26 into Codeception:master Jan 31, 2017

sergeyklay changed the title ~~入力値チェックの比較が不完全~~ Use strict comparison for onlyExpectedArguments Jan 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use strict comparison for onlyExpectedArguments #121

Use strict comparison for onlyExpectedArguments #121

wage-slave-montyakku commented Jan 30, 2017

Nitpick-CI Jan 30, 2017

wage-slave-montyakku Jan 30, 2017

Nitpick-CI Jan 30, 2017

wage-slave-montyakku Jan 30, 2017

Nitpick-CI Jan 30, 2017

Nitpick-CI Jan 30, 2017

wage-slave-montyakku Jan 30, 2017

Nitpick-CI Jan 30, 2017

sergeyklay Jan 30, 2017

Nitpick-CI Jan 30, 2017

sergeyklay Jan 30, 2017

wage-slave-montyakku commented Jan 31, 2017

sergeyklay commented Jan 31, 2017

DavertMik commented Jan 31, 2017

wage-slave-montyakku commented Feb 1, 2017

Use strict comparison for onlyExpectedArguments #121

Use strict comparison for onlyExpectedArguments #121

Conversation

wage-slave-montyakku commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wage-slave-montyakku commented Jan 31, 2017

sergeyklay commented Jan 31, 2017

DavertMik commented Jan 31, 2017

wage-slave-montyakku commented Feb 1, 2017