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

            $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

             $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

             $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

Use strict comparison for onlyExpectedArguments #121

Use strict comparison for onlyExpectedArguments #121

Uh oh!

Conversation

wage-slave-montyakku commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wage-slave-montyakku commented Jan 31, 2017

Uh oh!

sergeyklay commented Jan 31, 2017

Uh oh!

DavertMik commented Jan 31, 2017

Uh oh!

wage-slave-montyakku commented Feb 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants