チケット #791 (closed 改善提案: 修正済)

登録: 19 か月

最終更新: 11 か月

空白チェックの実装を統一

報告者: nanasess 担当者: nanasess
優先度: マイルストーン: EC-CUBE2.11.1
コンポーネント: その他 バージョン: 2.4.4
キーワード: 関係者:
修正済み:

説明

変数が空白かどうかをチェックするための処理が統一されていない

strlen($val) >= 1
strlen($val) < 1
strlen($val) == 0
strlen($val) > 0
empty($val)
!empty($val)

strlen の引数が配列の場合は Array という文字列を評価してしまうし, empty の引数に 0 が入った場合は false を返す.

SC_Utils::isBlank($mixed) などに統一し, 文字列や配列を透過的にチェックできるようにした方が良いと思われる

チケットの履歴

フォローアップ: ↓ 2   更新者: nanasess (18 か月 前)

  • 担当者 somebody から nanasess に変更されました
  • ステータスnew から assigned に変更されました。

version-2_5-dev r18768 で対応しました

↑ 1 への返信 ; フォローアップ: ↓ 3   更新者: Seasoft (17 か月 前)

version-2_5-dev r18768 で対応しました

trim() は不要だと思うのですが、いかがでしょうか?

strlen() や empty() での評価で実質的な問題が発生していないのであれば、trim() は不要と考えます。
trim() する必要がある場合、trim() をした後の値を引数に渡すべきだと考えます。

↑ 2 への返信 ; フォローアップ: ↓ 4   更新者: nanasess (17 か月 前)

Seasoft への返信

trim() は不要だと思うのですが、いかがでしょうか?

タブ文字や, 改行文字のみの場合は, 空白と見なさない方が良い感じでしょうか?

入力フォームなどで, 空白チェックをする場合, タブ文字や改行文字は, 空白と見なす場合が多く, 意図的に許容することは少ないのではないでしょうか? また, これらの文字が意図せず混入すると, 誤作動の原因にもなりかねません.

このような理由から, trim() で空白文字, タブ文字などを除去してからチェックするようにしましたが, いかがでしょうか?

↑ 3 への返信 ; フォローアップ: ↓ 6   更新者: Seasoft (17 か月 前)

タブ文字や, 改行文字のみの場合は, 空白と見なさない方が良い感じでしょうか?

そのように考えています。フロント機能の入力バリデーション専用ならば、空白とみなしても良いと思いますが、管理機能やシステム部分でも積極的(画一的)に利用したいので、便利さよりも正確さを求めます。

入力フォームなどで, 空白チェックをする場合, タブ文字や改行文字は, 空白と見なす場合が多く, 意図的に許容することは少ないのではないでしょうか?

入力フォームの空白チェックを意図するのであれば、SC_FormParam や SC_CheckError で行なうのが良いと考えます。たとえば、SC_FormParam#addParam の第4引数 ($convert) で trim するような実装があれば便利だとは思います。それで対処しきれないとき用に、SC_Utils#isBlankWithTrim のようなメソッドの提供はあっても良いと思います。

私も、PHP では strlen() を多用する実装者ですが、その主な理由は PHP で「$var = 0; var_dump($var == "");」が true であるといった「==」演算子の特殊な動作にあります。「strlen($var) == 0」に実際に求めるのは、VBの「var = ""」や Java の「(var == null || var.equals(""))」のような動作です。

フォローアップ: ↓ 7   更新者: Seasoft (17 か月 前)

if (!SC_Utils::isBlank($in, $greedy)) {

SC_Utils ではなく、SC_Utils_Ex では?

こんな核になりそうなメソッドを拡張されると怖いですが・・・

↑ 4 への返信 ; フォローアップ: ↓ 8   更新者: nanasess (17 か月 前)

Seasoft への返信

タブ文字や, 改行文字のみの場合は, 空白と見なさない方が良い感じでしょうか?

そのように考えています。フロント機能の入力バリデーション専用ならば、空白とみなしても良いと思いますが、管理機能やシステム部

それでは 第二引数の $greedy "貧欲"チェックオプションが true の場合のみ trim する仕様ではいかがでしょうか?

このオプションが false の場合は, 全角空白や, ネストした配列を評価しないので, strlen() に近い振舞いとなります

↑ 5 への返信 ; フォローアップ: ↓ 9   更新者: nanasess (17 か月 前)

Seasoft への返信

SC_Utils ではなく、SC_Utils_Ex では? こんな核になりそうなメソッドを拡張されると怖いですが・・・

継承先のクラスへ static で再帰した場合, 無限ループやメモリリークなどの懸念があったため, 自クラスへ再帰しています. 単なる検証不足ですが(汗)

この関数をオーバーライドした場合は, 単純に内部で SC_Utils_Ex を再帰させてやれば良いので, 現状の実装で問題ないのではないかと思います.

↑ 6 への返信 ; フォローアップ: ↓ 10   更新者: Seasoft (17 か月 前)

nanasess への返信

それでは 第二引数の $greedy "貧欲"チェックオプションが true の場合のみ trim する仕様ではいかがでしょうか? このオプションが false の場合は, 全角空白や, ネストした配列を評価しないので, strlen() に近い振舞いとなります

今のところ $greedy をどのように使い分けるか十分に把握できていませんが、少なくともデフォルトが現状の動作と同じと、移行しやすくて助かります。

あとは、配列が渡った場合に、例外エラーで落ちる関数も(別関数で良いと思いますが)あると便利かなとも思っています。こちらは、主にフォーム入力を意識した場合ですが、意図しなく配列だった場合、不正操作の懸念もあると考えました。これも、SC_FormParam とかで行なう方がスマートな気もするので、もう少し考えないとですが。

↑ 7 への返信 ; フォローアップ: ↓ 11   更新者: Seasoft (17 か月 前)

nanasess への返信

継承先のクラスへ static で再帰した場合, 無限ループやメモリリークなどの懸念があったため, 自クラスへ再帰しています. 単なる検証不足ですが(汗)

そういった懸念があるのですね。十分に把握できておらず恐縮です。

この関数をオーバーライドした場合は, 単純に内部で SC_Utils_Ex を再帰させてやれば良いので, 現状の実装で問題ないのではないかと思います.

個人的には、SC_Utils と SC_Utils_Ex が混在して、実装誤りか意図的なものか、後で見て分からなくなる事に懸念を感じています。原則をどちらかに決めて、例外はコメントを残すなど、カバーしておく必要があるのではないかと感じています。

↑ 8 への返信   更新者: nanasess (17 か月 前)

Seasoft への返信

今のところ $greedy をどのように使い分けるか十分に把握できていませんが、少なくともデフォルトが現状の動作と同じと、移行しやすくて助かります。

$greedy が true の場合は, ネストした配列や, 全角の空白もチェック対象となります.

あとは、配列が渡った場合に、例外エラーで落ちる関数も(別関数で良いと思いますが)あると便利かなとも思っています。こちらは、主にフォーム入力を意識した場合ですが、意図しなく配列だった場合、不正操作の懸念もあると考えました。これも、SC_FormParam とかで行なう方がスマートな気もするので、もう少し考えないとですが。

現在は, 第二引数が boolean ですが, 定数にして,

STRING_STRICT
厳密に文字列のみを検証する
GREEDY_CHECK
trimを行った上, ネストした配列や, 全角スペースも対象とする
NORMAL_CHECK
strlen() や empty() に近い振舞い

といった感じはどうでしょう?

定数名は要検討ですが.

↑ 9 への返信   更新者: nanasess (17 か月 前)

Seasoft への返信

個人的には、SC_Utils と SC_Utils_Ex が混在して、実装誤りか意図的なものか、後で見て分からなくなる事に懸念を感じています。原則をどちらかに決めて、例外はコメントを残すなど、カバーしておく必要があるのではないかと感じています。

そうですね. 原則は SC_Utils_Ex として, SC_Utils を使用する場合は, why なコメントつけておきます!

  更新者: nanasess (13 か月 前)

  • マイルストーンEC-CUBE2.5.0beta から EC-CUBE2.5.1(仮) に変更されました。

  更新者: nanasess (13 か月 前)

r19895 PHP5.3 未満で, 空白の配列を false と評価していたのを修正しました

  更新者: kotani (11 か月 前)

  • ステータスassigned から closed に変更されました。
  • 解決方法修正済 に設定されました。

実装済みのためクローズ

Note: チケットについてのヘルプは TracTickets を参照 して下さい。