Feelin' Kinda Strange

古来より、PHPの ‘is_numeric‘ 関数は鬼門とされております。

PHPのis_numeric関数は使うべきでないという話 - hnwの日記

先日見かけた、このエントリーにはコードレビューでの例にis_numericへの指摘がありました。

私がコードレビューの際に気をつけているコメントの書き方 - BASE開発チームブログ

最近私のほうは、PHPでのQAツール環境の土壌が豊富になってきたこともあり、言語やツール由来のハマリ所はCIでどんどん解決したほうが良いという見解からつい、このようなつぶやきをしております。

`is_numeric` を禁止するCS作って、preg_match('/\A[0-9]+\z/') なら `is_numeric`にするmutation作ればよそうなどと思ってしまったhttps://t.co/78OQUgndc1
— sasezaki (@sasezaki) 2019年6月13日

これは、

コーディングスタンダートチェックツールでis_numeric() を禁止する
- このCSチェックツールがpreg_matchで書き換えろと警告すればなおベター
preg_match でコード修正する
preg_match で数値チェックな正規表現だったら、テストパターンが足りているか、is_numericに変異させるmutatorで検知させる

のサイクルで、人によるレビューは減らせるよねという目論みです。

PHP でのmutation testingツールには、Humbugの後継とも言えるinfectionがあります。infectionの導入については、以下のエントリーを参照ください。 - PHP で mutation testing を試す - y_uti のブログ

infectionには多様なMutatorがあります。 - https://infection.github.io/guide/mutators.html#Regex

今回の特定の正規表現をPHPの組み込み関数に戻してみる、というのは現在のところありません。 (Preg match regex mutators by adeptofvoltron · Pull Request #388 · infection/infection というPRがありますが、こちらは正規表現そのもののマッチ方法を変えてみるもののようです )

そこで、エントリタイトルの通り preg_match('/\A[0-9]+\z/') だったら、‘is_numeric‘ にしてみる Mutatorを作ってみました。 - https://github.com/sasezaki/infection/commit/adf8dbe7b3d099ee5ad6afa3ae30fb2b5d9b0b99

デモ用のリポジトリを作ってみたので、そちらで動作確認できますhttps://github.com/sasezaki/infection_preg_match_is_numeric_example

CS チェックで is_numericを禁止する

phpcsをお使いの場合、phpcs.xml に以下の通りルールを追加することで、特定の関数を禁止できます

    <rule ref="Generic.PHP.ForbiddenFunctions">
        <properties>
            <property name="forbiddenFunctions" type="array">
                <element key="is_numeric" value="null"/>
            </property>
        </properties>
    </rule>

テストパターンを用意・実行する

さきほどのコードレビューについてのエントリに要件確認としてのテストがなかったのが気になりますが、それはさておき、電話番号チェックのテストを以下の通り作成してみます。

<?php 
class UtilTest extends TestCase
{
    public function testIs_phone_number()
    {
        $this->assertTrue(Util::is_phone_number('0312345678'));
    }
}

phpunitコマンドの実行結果

$ ./vendor/bin/phpunit
PHPUnit 8.2.1 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 193 ms, Memory: 4.00 MB

OK (1 test, 1 assertion)

はい。passします。

infectionでパターンの漏れを検出する

さきほどのテスト、もちろんパターンが不足しているので infectionを実行する (※ 現在の master HEADでの場合) と

phpdbg -qrr vendor/infection/bin/infection

$ cat infection.log
Escaped mutants:
================


1) /tmp/infection_preg_match_is_numeric_example/src/Util.php:12    [M] GreaterThan

--- Original
+++ New
@@ @@
     {
         $phone_number = str_replace('-', '', $phone_number);
         $length = strlen($phone_number);
-        if ($length < 10 || $length > 11) {
+        if ($length < 10 || $length >= 11) {
             return false;
         }
         return preg_match('/\\A[0-9]+\\z/', $phone_number);
     }

2) /tmp/infection_preg_match_is_numeric_example/src/Util.php:12    [M] LogicalOr

--- Original
+++ New
@@ @@
     {
         $phone_number = str_replace('-', '', $phone_number);
         $length = strlen($phone_number);
-        if ($length < 10 || $length > 11) {
+        if ($length < 10 && $length > 11) {
             return false;
         }
         return preg_match('/\\A[0-9]+\\z/', $phone_number);
     }
Timed Out mutants:
==================

Not Covered mutants:
====================


1) /tmp/infection_preg_match_is_numeric_example/src/Util.php:13    [M] FalseValue

--- Original
+++ New
@@ @@
         $phone_number = str_replace('-', '', $phone_number);
         $length = strlen($phone_number);
         if ($length < 10 || $length > 11) {
-            return false;
+            return true;
         }
         return preg_match('/\\A[0-9]+\\z/', $phone_number);
     }
 }

とテストパターンに漏れがあることが確認できます。それゆえ、カバレッジ100になるようなテストパターンを以下の通り用意すると

        $this->assertTrue(Util::is_phone_number('0312345678'));
        $this->assertFalse(Util::is_phone_number('031234567'));
        $this->assertTrue(Util::is_phone_number('09012345678'));
        $this->assertFalse(Util::is_phone_number('090123456789'));

$ phpdbg -qrr ./vendor/infection/bin/infection
You are running Infection with phpdbg enabled.
     ____      ____          __  _
    /  _/___  / __/__  _____/ /_(_)___  ____
    / // __ \/ /_/ _ \/ ___/ __/ / __ \/ __ \
  _/ // / / / __/  __/ /__/ /_/ / /_/ / / / /
 /___/_/ /_/_/  \___/\___/\__/_/\____/_/ /_/

Running initial test suite...

PHPUnit version: 8.2.1

    7 [============================] < 1 sec

Generate mutants...

Processing source code files: 1/1
Creating mutated files and processes: 7/7
.: killed, M: escaped, S: uncovered, E: fatal error, T: timed out

.......                                              (7 / 7)

7 mutations were generated:
       7 mutants were killed
       0 mutants were not covered by tests
       0 covered mutants were not detected
       0 errors were encountered
       0 time outs were encountered

Metrics:
         Mutation Score Indicator (MSI): 100%
         Mutation Code Coverage: 100%
         Covered Code MSI: 100%

Please note that some mutants will inevitably be harmless (i.e. false positives).

と infectionでの実行もパスします。

数値チェック正規表現用のMutatorを実行する

さきほどのはinfection現在のmaster HEADなバージョンでの場合でした。これを今回の数値チェック正規表現用のMutatorを追加したものでテストをかけてみます。 ~~以下は、その sasezaki/preg_match_is_numericブランチ版にてコマンドを走らせた場合です~~(8/17追記、別途サードパーティーMutatorを追加していするものにサンプルを修正しました)

$ phpdbg -qrr ./vendor/bin/infection
You are running Infection with phpdbg enabled.
     ____      ____          __  _
    /  _/___  / __/__  _____/ /_(_)___  ____
    / // __ \/ /_/ _ \/ ___/ __/ / __ \/ __ \
  _/ // / / / __/  __/ /__/ /_/ / /_/ / / / /
 /___/_/ /_/_/  \___/\___/\__/_/\____/_/ /_/

Running initial test suite...

PHPUnit version: 8.2.1

    7 [============================] < 1 sec

Generate mutants...

Processing source code files: 1/1
Creating mutated files and processes: 8/8
.: killed, M: escaped, S: uncovered, E: fatal error, T: timed out

......M.                                             (8 / 8)

8 mutations were generated:
       7 mutants were killed
       0 mutants were not covered by tests
       1 covered mutants were not detected
       0 errors were encountered
       0 time outs were encountered

Metrics:
         Mutation Score Indicator (MSI): 87%
         Mutation Code Coverage: 100%
         Covered Code MSI: 87%

Please note that some mutants will inevitably be harmless (i.e. false positives).

とdetectedがされ、ログを見ると

$ cat infection.log
Escaped mutants:
================


1) /tmp/infection_preg_match_is_numeric_example/src/Util.php:16    [M] PregMatchIsNumeric

--- Original
+++ New
@@ @@
         if ($length < 10 || $length > 11) {
             return false;
         }
-        return preg_match('/\\A[0-9]+\\z/', $phone_number);
+        return is_numeric($phone_number);
     }
 }

Timed Out mutants:
==================

Not Covered mutants:
====================

と is_numericに変異させた場合でもテストがパスできることからパターンの漏れがあることが分かります。

よって、さきほどのコードレビュー記事にあるような少数点ありの数値文字列のパターンを用意してみます。

$this->assertFalse(Util::is_phone_number('0.12345678'));

すると、

$ phpdbg -qrr ./vendor/bin/infection
You are running Infection with phpdbg enabled.
     ____      ____          __  _
    /  _/___  / __/__  _____/ /_(_)___  ____
    / // __ \/ /_/ _ \/ ___/ __/ / __ \/ __ \
  _/ // / / / __/  __/ /__/ /_/ / /_/ / / / /
 /___/_/ /_/_/  \___/\___/\__/_/\____/_/ /_/

Running initial test suite...

PHPUnit version: 8.2.1

    5 [============================] < 1 sec

Generate mutants...

Processing source code files: 1/1
Creating mutated files and processes: 8/8
.: killed, M: escaped, S: uncovered, E: fatal error, T: timed out

........                                             (8 / 8)

8 mutations were generated:
       8 mutants were killed
       0 mutants were not covered by tests
       0 covered mutants were not detected
       0 errors were encountered
       0 time outs were encountered

Metrics:
         Mutation Score Indicator (MSI): 100%
         Mutation Code Coverage: 100%
         Covered Code MSI: 100%

Please note that some mutants will inevitably be harmless (i.e. false positives).

となり、"数値チェック"のパターンに桁数以外でのチェックが入っているかの整数としての確認パターンを加えられるようになりました。

興味を持たれた方は、このMutator含めてinfection 試してみて下さい。

is_numeric用Mutator ブランチ GitHub - sasezaki/infection at preg_match_is_numeric
実行デモ github.com

おまけ - 数値チェック正規表現のゆれ

今回の正規表現チェックは、単に'\A[0-9]+\z' === $regex にて検出するようにしていますが、その場合、数値チェックの正規表現で多く出回っている/^[0-9]+$/ をコピペされたらどうするんだ！？とお思いの方もいらっしゃるはずです。

安心してください。

"正規表現によるバリデーションでは ^ と $ ではなく" 、それっぽいPHPStanルール作ったhttps://t.co/klNC4A6RRj pic.twitter.com/tzSETjBCGK
— sasezaki (@sasezaki) 2019年6月1日

https://github.com/sasezaki/angry-regex