Ticket #791 (closed 改善提案: 修正済)

Opened 10 years ago

Last modified 10 years ago

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

Reported by: nanasess Owned by: nanasess
Priority: Milestone: EC-CUBE2.11.1
Component: その他 Version: 2.4.4
Keywords: Cc:
修正済み:

Description

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

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) などに統一し, 文字列や配列を透過的にチェックできるようにした方が良いと思われる

Change History

comment:1 follow-up: ↓ 2 Changed 10 years ago by nanasess

  • Owner changed from somebody to nanasess
  • Status changed from new to assigned

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

comment:2 in reply to: ↑ 1 ; follow-up: ↓ 3 Changed 10 years ago by Seasoft

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

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

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

comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 10 years ago by nanasess

Seasoft への返信

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

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

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

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

comment:4 in reply to: ↑ 3 ; follow-up: ↓ 6 Changed 10 years ago by Seasoft

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

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

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

入力フォームの空白チェックを意図するのであれば、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(""))」のような動作です。

comment:5 follow-up: ↓ 7 Changed 10 years ago by Seasoft

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

SC_Utils ではなく、SC_Utils_Ex では?

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

comment:6 in reply to: ↑ 4 ; follow-up: ↓ 8 Changed 10 years ago by nanasess

Seasoft への返信

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

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

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

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

comment:7 in reply to: ↑ 5 ; follow-up: ↓ 9 Changed 10 years ago by nanasess

Seasoft への返信

SC_Utils ではなく、SC_Utils_Ex では?

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

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

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

comment:8 in reply to: ↑ 6 ; follow-up: ↓ 10 Changed 10 years ago by Seasoft

nanasess への返信

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

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

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

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

comment:9 in reply to: ↑ 7 ; follow-up: ↓ 11 Changed 10 years ago by Seasoft

nanasess への返信

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

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

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

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

comment:10 in reply to: ↑ 8 Changed 10 years ago by nanasess

Seasoft への返信

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

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

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

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

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

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

定数名は要検討ですが.

comment:11 in reply to: ↑ 9 Changed 10 years ago by nanasess

Seasoft への返信

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

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

comment:12 Changed 10 years ago by nanasess

  • Milestone changed from EC-CUBE2.5.0beta to EC-CUBE2.5.1(仮)

comment:13 Changed 10 years ago by nanasess

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

comment:14 Changed 10 years ago by kotani

  • Status changed from assigned to closed
  • Resolution set to 修正済

実装済みのためクローズ

Note: See TracTickets for help on using tickets.