Bug(バグ) #1595
完了ProfileForm で日付型のプロフィール項目の最大値・最小値の入力欄に now などの strtotime() が解釈できる文字列を入力すると、そのプロフィール項目を保存した時点の日付が DB に保存されてしまう
100%
説明
ProfileForm で日付型のプロフィール項目の最大値・最小値の入力欄に now などの strtotime() が解釈できる文字列を入力すると、そのプロフィール項目を保存した時点の日付が DB に保存されてしまう。
この問題は #930 の 4e1f2665 の変更で混入したもの。
たとえば日付型プロフィールの最大値を today や now などにして保存すると、 2010-09-17 といった入力時点の日付で登録されてしまう。これらの文字列はそのまま DB に保存し、メンバーがプロフィールを入力した際に now や 2010-09-17 や next Sunday といった文字列をその時点での日付に変換した上で、メンバーの入力値と比較するのが正しい挙動である。
Kousuke Ebihara さんが約14年前に更新
- 対象バージョン を OpenPNE 3.6beta6 にセット
たとえば、誕生日にプリセットプロフィールを使っていなかった頃の SNS からアップデートした場合にこの問題の影響を影響を受けます
Masato Nagasawa さんが13年以上前に更新
- ステータス を New(新規) から Accepted(着手) に変更
- 担当者 を Masato Nagasawa にセット
Kousuke Ebihara さんが13年以上前に更新
- 優先度 を Normal(通常) から High(高め) に変更
#930 の変更を取り込んで以降のバージョン、およびそのバックポートによって後方互換性が壊れています。優先度を上げて取り組みたいです。
Kousuke Ebihara さんが13年以上前に更新
http://redmine.openpne.jp/issues/940#note-26 にてこのチケットも含めた現状の実装の問題について述べています。
Minoru Takai さんが13年以上前に更新
- ステータス を Accepted(着手) から Pending Review(レビュー待ち) に変更
- 進捗率 を 0 から 50 に変更
更新履歴 9072bbd231dc28faaf9b010c0622fe7c99639d89 で適用されました。
Minoru Takai さんが13年以上前に更新
note-9 で lib/form/doctrine/ProfileForm.class.php を修正しました。 #930 ではテンプレートとフォームクラスを修正していますが、 #930 でのフォームクラスの修正はほぼ取り消しました。
以下に修正内容の意味を示しておきます。
+ $this->mergePostValidator(new sfValidatorCallback(array('callback' => array('ProfileForm', 'validateValueMin')))); + $this->mergePostValidator(new sfValidatorCallback(array('callback' => array('ProfileForm', 'validateValueMax')))); #(*1) value_min と value_max に対して別々にバリデータをセットしました。 # 別々にしたのは最小値と最大値が同時にエラーだった場合に両方ともエラーメッセージを出すためです。 # ※このような場合にバリデータを別々にせずに対応することが可能なのか知りません。 $this->mergePostValidator(new sfValidatorCallback(array('callback' => array('ProfileForm', 'validateName')))); $this->setValidator('default_public_flag', new sfValidatorChoice(array('choices' => array_keys(Doctrine::getTable('Profile')->getPublicFlags())))); $this->setValidator('value_min', new sfValidatorPass()); : $this->widgetSchema->setHelp('is_public_web', 'Anyone in the world may view member profiles'); } - public function bind($params) + static public function validateValue($validator, $values, $valueKey) #(*2) (*1) で呼ばれるバリデータの内部で呼んでいます。 { - if ($params['form_type'] === 'input' || $params['form_type'] === 'textarea') + $options = array('required' => false); + $validator = null; + + switch ($values['form_type']) { - $validator = new sfValidatorInteger(array('required' => false)); - $this->setValidator('value_min', $validator); - $validator = new sfValidatorInteger(array('required' => false)); - $this->setValidator('value_max', $validator); + case 'input': + case 'textarea': + $validator = new sfValidatorInteger($options); + break; + case 'date': + $validator = new sfValidatorDate($options); + break; + default: + break; // Do nothing. } - elseif ($params['form_type'] === 'date') + + if (null !== $validator) { - $validator = new opValidatorDate(array('required' => false)); - $this->setValidator('value_min', $validator); - $validator = new opValidatorDate(array('required' => false)); - $this->setValidator('value_max', $validator); + try + { + $validator->clean($values[$valueKey]); + } + catch (Exception $e) + { + throw new sfValidatorErrorSchema($validator, array($valueKey => new sfValidatorError($validator, 'invalid'))); + } #(*3) $validator->clean() の結果を用いてバリデートしています。 # clean() では不正入力時に例外が投げられるので catch して sfValidatorErrorSchema で例外を投げ直しました。 # これにより、最小値・最大値の入力値の箇所にエラーメッセージを表示できるようにしました( #930 の修正内容)。 } - elseif ($params['value_min'] || $params['value_max']) + elseif ($values[$valueKey]) { throw new sfValidatorError($validator, 'invalid'); } #(*4) この例外を投げる理由を把握していませんが、もともとの記述を引き継いでいます。 - return parent::bind($params); + return $values; + } + + static public function validateValueMin($validator, $values) + { + return self::validateValue($validator, $values, 'value_min'); + } + + static public function validateValueMax($validator, $values) + { + return self::validateValue($validator, $values, 'value_max'); } static public function validateName($validator, $values)
- ProfileForm::bind() メソッドを削除しました(これを残したまま、相対的な日付を保存させる方法が分かりませんでした)。
- #930 修正前に用いていたバリデータ advancedValidator() メソッドを元に、エラーメッセージを「最小値」「最大値」個別に表示するようにしました。
- validateValueMin(), validateValueMax() をそれぞれコールさせ、その中で validateValue() を呼ぶようにしていますが、これがパフォーマンス的に好ましくなかったり、よりスマートな方法があればフィードバックを頂きたいです。
Rimpei Ogawa さんが13年以上前に更新
- ステータス を Pending Review(レビュー待ち) から Pending Testing(テスト待ち) に変更
- 進捗率 を 50 から 70 に変更
修正内容確認しました。問題はありませんでした。
advancedValidator() と比較して、sfValidatorInteger を利用した場合に clean() の結果を使用していない点が若干気になりましたが、intval() をしているかどうかの違いなのでこの後の処理には影響はないと判断しました。
また、validateValueMin(), validateValueMax() に分ける方法はとくに問題ないと思いました(よりスマートな方法を思いつきませんでした:無理に1つにまとめても例外処理が面倒だったりするのでこの方法でいいかなと思います)。
Minoru Takai さんが13年以上前に更新
- ステータス を Pending Testing(テスト待ち) から Rejected(差し戻し) に変更
- 進捗率 を 70 から 50 に変更
note-10 で実装をしましたが、 note-11 のレビューを見て、改めて自分でも見なおしたところ、以前の advancedValidator() あるいは #930 での書き換え後の bind() と、今回の修正で処理内容が同様でない実装となっていることに気付いたので修正内容を見直します。
- 今回の修正の一部:
+ case 'date': + $validator = new sfValidatorDate($options); + break;
具体的には、日付型のバリデートに関して、このチケットで扱っている管理画面側で、以前は opValidatorDate を用いていましたが、今回の修正で sfValidatorDate を用いてしまっています。なお、メンバー側の日付入力部分では、以前も現在も opValidatorDate を用いています。これが異なっていることは適切ではないはずなので、 opValidatorDate を用いるように修正します。
t930 修正前と修正後の差分 を改めて確認しました。 note-11 での指摘について、「advancedValidator() と比較して、sfValidatorInteger を利用した場合に clean() の結果を使用していない点」は気づいていませんでした。行ったほうが適切なようであれば見直しと同時にこれを追加しようと思います。
Rimpei Ogawa さんが13年以上前に更新
Minoru Takai は書きました:
具体的には、日付型のバリデートに関して、このチケットで扱っている管理画面側で、以前は opValidatorDate を用いていましたが、今回の修正で sfValidatorDate を用いてしまっています。なお、メンバー側の日付入力部分では、以前も現在も opValidatorDate を用いています。これが異なっていることは適切ではないはずなので、 opValidatorDate を用いるように修正します。
これはレビュー側で見落としていました。修正方針問題ないと思います。
t930 修正前と修正後の差分 を改めて確認しました。 note-11 での指摘について、「advancedValidator() と比較して、sfValidatorInteger を利用した場合に clean() の結果を使用していない点」は気づいていませんでした。行ったほうが適切なようであれば見直しと同時にこれを追加しようと思います。
通常のフォームクラスを利用した処理では save() 時に利用される値はバリデーターの clean() の返り値となっており、今回の箇所に関しては opValidatorDate のみ特別にこの clean() の返り値を利用しないような変更をしているものだという認識です。
sfValidatorInteger の clean() 処理は単純で副作用も予測できる範囲なので問題なしとしましたが、他のフォームフィールドとの扱いを揃えるという意味では、特別な理由がない限りは clean() の結果を利用するようにした方がよいのではないかと思います。
Minoru Takai さんが13年以上前に更新
- ステータス を Accepted(着手) から Pending Review(レビュー待ち) に変更
更新履歴 afee7267847f2350da4accd21e181a6c1729be3b で適用されました。
Minoru Takai さんが13年以上前に更新
note-12, note-14 の内容について修正を行いました。
- a932c178 (fixes #1595) uses opValidatorDate (from sfValidatorDate) for date value in ProfileForm
- 管理画面のプロフィール設定での日付型バリデータに opValidatorDate を用いるようにし、これまでと同様に opValidatorDate でない場合に限って clean() した値で入力値を更新するようにしました。
- afee7267 (fixes #1595) fixed message of about date-format.
- 翻訳ファイルとテンプレートの記述が不一致で翻訳されていない問題を修正しました。また、このチケットで修正した本来あるべき仕様(相対的な日付が入力できる仕様)についてのメッセージを追加しました。
note-11 で既にレビューされていますが、この修正内容と、改めて全体のレビューを再度お願いしたいと思います。
Maki Takahashi さんが約13年前に更新
- ステータス を Pending Review(レビュー待ち) から Rejected(差し戻し) に変更
Minoru Takai さんが約13年前に更新
- ステータス を Rejected(差し戻し) から Pending Review(レビュー待ち) に変更
更新履歴 b51d54adc43ef3dc945236b714e692b96f9da74e で適用されました。
Minoru Takai さんが約13年前に更新
note-20 で修正しました。
- a932c178
- lib/form/doctrine/ProfileForm.class.phpの124行目 if文の記述がコーディング規約に反しています(終了の波括弧が改行されていない)
if (!($validator instanceof opValidatorDate)) {
修正しました。
- afee7267
- 追加された「本来あるべき仕様(相対的な日付が入力できる仕様)についてのメッセージ」の英文が不自然なようです
If you input "now," 〜(省略)nowの後にあるカンマは"の外にくるのが正しいのではないでしょうか。
引用符の外にカンマを打つべきかどうかは英文法に依り、「カンマ 引用符」などのキーワードでウェブ検索すると記事が出てくると思いますが、一般的に「(引用符は二重引用符を用いて)カンマはその外に打つべきである」と判断するのは根拠が足りないと思います。
この指摘について、本来は根拠を示した上でカンマを中と外のどちらに打つべきかを決めたいところですが、的確な情報が見つけ出せなかったので今回はこの議論をせずに、引用符で括ったワードの直後にカンマが来ないような文章に変更することにしました。 (ref. If you input "now" as a date, it means ...)
ついでに、メッセージについて日本語訳も少し修正しました。
Maki Takahashi さんが約13年前に更新
- ステータス を Pending Review(レビュー待ち) から Pending Testing(テスト待ち) に変更
- 進捗率 を 50 から 70 に変更
b51d54ad 修正確認いたしました。ありがとうございます。
redmine上のapps/pc_backend/i18n/messages.ja.xmlのdiff表示が(safari5.1にて)文字化けしている部分がありますが
diff表示のみ文字化けしているだけのようです。
Yuma Sakata さんが約13年前に更新
- ステータス を Pending Testing(テスト待ち) から Fixed(完了) に変更
- 進捗率 を 70 から 100 に変更
テストOKです。