Bug(バグ) #2882
完了opValidatorDate で「年」に 0 を入力した場合に空値として認識される
0%
説明
現象¶
opValidatorDate を使用しているフォーム(コミュニティイベントの作成画面など)で、日付を入力する項目の「年」に 0
を入力すると空値として扱われる。
- コミュニティイベントの作成画面の場合、「募集期日」の「年」に
0
と入力された状態(「月」「日」は入力しない)で送信しても「正しくありません。」と表示されずにイベントの作成が完了する。
原因¶
lib/validator/opValidatorDate.class.php の 100 行目付近
// if one date value is empty, all others must be empty too
$empties =
(!isset($value['year']) || !$value['year'] ? 1 : 0) +
(!isset($value['month']) || !$value['month'] ? 1 : 0) +
(!isset($value['day']) || !$value['day'] ? 1 : 0);
この判定で、「年」($value['year']
) に "0"
が入力されると !isset($value['year']) || !$value['year']
が true
となり空値と判定されることが原因。
修正内容¶
修正内容を記入
kaoru n さんがほぼ12年前に更新
原因は、上記記述内容ではなく(opValidatorDateは使用していない)、以下の記述内容にありました。
入力チェックに使用しているクラスsfValidatorDate内で入力値のEmptyチェックを行っています(sfValidatorDateのisEmpty)が、与えられた引数がarrayの場合に「array_filter」という関数を使用しています。
この関数は(http://php.net/manual/ja/function.array-filter.phpより)
コールバック関数が与えられなかった場合、 input のエントリの中で FALSE と等しいもの (boolean への変換 を参照ください) がすべて削除されます。
ということで、年に入力された0がfalse扱いされ削除されます。
その後Emptyチェックを行うため、年に0のみが入力された場合、Emptyと判定され必須項目ではないためエラーチェックに引っかかりません。
kaoru n さんがほぼ12年前に更新
- ステータス を Accepted(着手) から Pending Review(レビュー待ち) に変更
- 担当者 を削除 (
kaoru n) - 進捗率 を 0 から 50 に変更
https://github.com/openpne/OpenPNE3/pull/62
プルリクエストしました。
Yuya Watanabe さんがほぼ12年前に更新
sfValidatorBase.class.php では値が下記の 3 つのいずれかになる場合に isEmpty() が true となる.
特別な値 NULL (値がセットされていない変数を含む) 空の文字列 要素の数がゼロである 配列
sfValidatorDate.class.php では上記に加えて,配列の中の物が下記のものも isEmpty() が true となる値である.
boolean の FALSE integer の 0 (ゼロ) float の 0.0 (ゼロ) 文字列の "0" 空のタグから作成された SimpleXML オブジェクト
PHP: 論理型 (boolean) - Manual
http://php.net/manual/ja/language.types.boolean.php#language.types.boolean.casting
わざわざ isEmpty() をオーバーライドしているのは, sfValidatorBase#isEmpty() が単項目のみを考慮しているため.
下記コミットで配列で渡された場合を考慮している.
http://trac.symfony-project.org/changeset/7081
ただし,上記コミットのテストコードを見て分かる通り(下記に記述),それぞれの中身が null, '', array() 以外のものであることを考慮していない.つまり,動作としては正しいが過剰にフィルタしているという推測ができる.このコミットが5年前(2013/01/08 現在)であり,それ以降 isEmpty() については修正がされていないため sfValidatorDate のバグとして Symfony 側を修正するという手もあるが,ここでは議論しないこととする.
158 // required 159 $v = new sfValidatorDate(); 160 foreach (array( 161 array('year' => '', 'month' => '', 'day' => ''), 162 array('year' => null, 'month' => null, 'day' => null), 163 '', 164 null, 165 ) as $input) 166 { 167 try 168 { 169 $v->clean($input); 170 $t->fail('->clean() throws an exception if the date is empty and required is true'); 171 } 172 catch (sfValidatorError $e) 173 { 174 $t->pass('->clean() throws an exception if the date is empty and required is true'); 175 } 176 }
sfValidatorDate#isEmpty() が 0 をどう扱うかについて言明していないことを考慮すると, opValidatorDate#isEmpty() によって入力がないかどうかを調べる際に 0 を空入力としないようにする修正は妥当であると思われる.ただし, プルリクエストのように sfValidatorBase#isEmpty() をそのまま使ってしまう場合, $value の値が配列である場合にそれぞれの値が空入力であるかどうかという点について考慮されなくなってしまうため間違いである.
また, year のみを考慮しているが現実としてはこの値を西暦とすると紀元前など正の整数以外の年が存在することを鑑みた上で 0 より大きい値のみを受け付けるという仕様として認識できるが,月および日については 0 月 8 日や 1 月 0 日などは存在しないため、むしろ月および日について修正を行う必要があるのではないかという考え方もできる.
別件¶
下記のようなコードが合った場合の出力として, isEmpty() が true か false かによって返ってくる型が違う.これは sfValidatorBase の中で isEmpty() が true の場合は getEmptyValue() によって返ってくる値をそのまま返し, false の場合は sfValidatorDate の処理を通って返ってくるという違いが発生するため.仕様といえば仕様だが直感的ではなさそう.
$validator = new opValidatorDate(); $validator->setOption('empty_value', new DateTime('1989-01-08')); var_dump($validator->clean(array('year' => '', 'month' => '', 'day' => ''))); var_dump($validator->clean(array('year' => '', 'month' => '', 'day' => '', 'hour' => '10')));
出力
object(DateTime)#23 (3) { ["date"]=> string(19) "1989-01-08 00:00:00" ["timezone_type"]=> int(3) ["timezone"]=> string(10) "Asia/Tokyo" } string(10) "1989-01-08"
別件2¶
lib/vendor/symfony/test/unit/validator/sfValidatorDateTest.php より,timezone についてのテストコードを拝借してきて opValidatorDate に適用してみたときにテストがパスしない.sfValidatorDate に変更すると通る.
1 <?php 2 3 include_once dirname(__FILE__) . '/../../bootstrap/unit.php'; 4 5 $t = new lime_test(1, new lime_output_color()); 6 7 $v = new opValidatorDate(); 8 9 $t->diag('opValidatorDate'); 10 11 // timezones 12 $defaultTimezone = new DateTimeZone(date_default_timezone_get()); 13 $otherTimezone = new DateTimeZone('US/Pacific'); 14 if ($defaultTimezone->getOffset(new DateTime()) == $otherTimezone->getOffset(new DateTime())) 15 { 16 $otherTimezone = new DateTimeZone('US/Eastern'); 17 } 18 19 $date = new DateTime('2000-01-01T00:00:00-00:00'); 20 $date->setTimezone($otherTimezone); 21 $v->setOption('with_time', true); 22 $clean = $v->clean($date->format(DATE_ATOM)); 23 24 $date->setTimezone($defaultTimezone); 25 $t->is($clean, $date->format('Y-m-d H:i:s'), '->clean() respects incoming and default timezones');
$ php opValidatorDateTestForTimezone.php not ok 1 - ->clean() respects incoming and default timezones # Failed test (./opValidatorDateTestForTimezone.php at line 25) # got: '1999-12-31 16:00:00' # expected: '2000-01-01 09:00:00'
Yuya Watanabe さんがほぼ12年前に更新
- ステータス を Pending Review(レビュー待ち) から Accepted(着手) に変更
- 進捗率 を 50 から 0 に変更
コードからみる仕様¶
付録 B - バリデーター (1_2) - Symfony
http://symfony.com/legacy/doc/forms/1_2/ja/B-Validators#chapter_b_sub_sfvalidatordate
- 次のキーで構成される配列: year、month、day、hour、 minute と second
- すべて数字
- year は 4 桁以内の数字
- year, month, day は all or nothing
- checkdate() が true になるような (year, month, date) の組み合わせがセットされている.
- with_time が指定されている場合は (hour), (hour, minute), (hour, minute, second) の組み合わせがセットされている.
- 提供されるのであれば date_format 正規表現にマッチする文字列
- PHP の strtotime() 関数によって解析できる文字列
- タイムスタンプを表す整数
- required が false かつ isEmpty() が false である場合は empty_value は strtotime() 関数によって解析できる文字列か date('YmdHis', $value) で解析できる数字列でなければならない.
opValidaterDate
- 次のキーで構成される配列: year、month、day、hour、 minute と second
- すべて数字
- year は 4 桁以内の数字
- year, month, day は all or nothing
- checkdate() が true になるような (year, month, date) の組み合わせがセットされている.
- with_time が指定されている場合は (hour), (hour, minute), (hour, minute, second) の組み合わせがセットされている.
- 提供されるのであれば date_format 正規表現にマッチする文字列
- PHP の strtotime() 関数によって解析できる文字列
- required が false かつ isEmpty() が false である場合は empty_value は DateTime オブジェクトでなければならない
- opValidaterDate は入力にタイムスタンプを許容しない
- opValidaterDate は empty_value に DateTime を用いる必要がある
isEmpty について¶
意図としては任意の値が空ではない(0 は空でないとする)状態を考えたいので下記のようなものが考えられる.
function isEmpty($value) { if (!is_array($value)) { return parent::isEmpty($value); } foreach ($value as $v) { if (!in_array($v, array(null, '', array()), true)) { return false; } } return true; }
setTimezone の挙動について¶
sfValidatorDate と opValidatorDate の挙動に差があるが,今回は考慮しないこととする.
修正方針¶
上記 isEmpty のコードを用いて空判定を行う. つまり, 0 は空文字列ではないという扱い.
また,タイムスタンプを許容しないことおよび empty_value が DateTime であるべきということをコメントで明記しておく.
Yuya Watanabe さんがほぼ12年前に更新
下記観点のテストケースを追加した結果を示しておく.
- (boolean) として扱う場合に false となるもので 空として扱うものと扱わないもの
- required が true の場合と false の場合
- isEmpty が true の場合と false の場合
- '',
- null,
- false,
- array()
- new SimpleXMLElement('<hoge />')
- 0
- '0'
$ prove --exec php opValidatorDateTest.php opValidatorDateTest.php .. Failed 10/80 subtests Test Summary Report ------------------- opValidatorDateTest.php (Wstat: 0 Tests: 76 Failed: 6) Failed tests: 58, 60, 68-69, 75-76 Parse errors: Bad plan. You planned 80 tests but ran 76. Files=1, Tests=76, 0 wallclock secs ( 0.04 usr 0.01 sys + 0.43 cusr 0.06 csys = 0.54 CPU) Result: FAIL
Yuya Watanabe さんがほぼ12年前に更新
浮動小数点を空入力として扱わないとしてテストを追加.
手元の環境ですべてのテストにパスすることを確認.
$ prove --exec php opValidatorDateTest.php opValidatorDateTest.php .. ok All tests successful. Files=1, Tests=84, 1 wallclock secs ( 0.04 usr 0.01 sys + 0.43 cusr 0.06 csys = 0.54 CPU) Result: PASS
Yuya Watanabe さんがほぼ12年前に更新
- ステータス を Accepted(着手) から Pending Review(レビュー待ち) に変更
- 進捗率 を 0 から 50 に変更
更新履歴 54730f08546bf98f8886cc0941b76ebfab24e086 で適用されました。
Kousuke Ebihara さんがほぼ12年前に更新
- ステータス を Pending Review(レビュー待ち) から Pending Testing(テスト待ち) に変更
- 進捗率 を 50 から 70 に変更