Project

General

Profile

Bug(バグ) #1595

ProfileForm で日付型のプロフィール項目の最大値・最小値の入力欄に now などの strtotime() が解釈できる文字列を入力すると、そのプロフィール項目を保存した時点の日付が DB に保存されてしまう

Added by Kousuke Ebihara almost 9 years ago. Updated over 3 years ago.

Status:
Fixed(完了)
Priority:
High(高め)
Assignee:
Target version:
Start date:
2010-09-17
Due date:
% Done:

100%

3.6 で発生するか:
Yes
3.8 で発生するか:
Unknown (未調査)

Description

ProfileForm で日付型のプロフィール項目の最大値・最小値の入力欄に now などの strtotime() が解釈できる文字列を入力すると、そのプロフィール項目を保存した時点の日付が DB に保存されてしまう。

この問題は #930 の 4e1f2665 の変更で混入したもの。

たとえば日付型プロフィールの最大値を today や now などにして保存すると、 2010-09-17 といった入力時点の日付で登録されてしまう。これらの文字列はそのまま DB に保存し、メンバーがプロフィールを入力した際に now や 2010-09-17 や next Sunday といった文字列をその時点での日付に変換した上で、メンバーの入力値と比較するのが正しい挙動である。


Related issues

Related to OpenPNE 3 - Backport(バックポート) #1706: ProfileForm で日付型のプロフィール項目の最大値・最小値の入力欄に now などの strtotime() が解釈できる文字列を入力すると、そのプロフィール項目を保存した時点の日付が DB に保存されてしまう Fixed(完了) 2010-09-17
Related to OpenPNE 3 - Bug(バグ) #940: プロフィール項目の日付やテキストの最小値を最大値より大きくして設定できてしまう Pending Fixing(修正待ち) 2010-04-05
Related to OpenPNE 3 - Bug(バグ) #837: プロフィール項目設定の文字数制限でmax=0,min=0と設定できてしまう Fixed(完了) 2010-03-11
Related to OpenPNE 3 - Bug(バグ) #1937: 日付のプロフィール項目で、管理画面で指定した上限の日が弾かれてしまう Fixed(完了)
Related to OpenPNE 3 - Backport(バックポート) #2164: ProfileForm で日付型のプロフィール項目の最大値・最小値の入力欄に now などの strtotime() が解釈できる文字列を入力すると、そのプロフィール項目を保存した時点の日付が DB に保存されてしまう Fixed(完了) 2011-06-09
Related to OpenPNE 3 - Bug(バグ) #930: the error of the minimum value/maximum of the form type is not displayed in the edit page to the profile item. (プロフィール項目編集画面フォームタイプの最小値/最大値のエラー表示がされない) Fixed(完了) 2010-04-05

Associated revisions

Revision 9072bbd2 (diff)
Added by Minoru Takai almost 8 years ago

(fixed #1595) revised to store an inputted value even if it is a relative date.

Revision a932c178 (diff)
Added by Minoru Takai almost 8 years ago

(fixes #1595) uses opValidatorDate (from sfValidatorDate) for date value in ProfileForm

Revision afee7267 (diff)
Added by Minoru Takai almost 8 years ago

(fixes #1595) fixed message of about date-format.

Revision b51d54ad (diff)
Added by Minoru Takai almost 8 years ago

(fixes #1595) fixed for coding standard, and changed message of about date-format.

History

#1 Updated by Kousuke Ebihara almost 9 years ago

  • Target version set to OpenPNE 3.6beta6

たとえば、誕生日にプリセットプロフィールを使っていなかった頃の SNS からアップデートした場合にこの問題の影響を影響を受けます

#2 Updated by Kousuke Ebihara almost 9 years ago

  • Target version changed from OpenPNE 3.6beta6 to OpenPNE 3.7.0

#3 Updated by Masato Nagasawa over 8 years ago

  • Status changed from New(新規) to Accepted(着手)
  • Assignee set to Masato Nagasawa

#4 Updated by Masato Nagasawa about 8 years ago

  • Status changed from Accepted(着手) to New(新規)

#5 Updated by Masato Nagasawa about 8 years ago

  • Assignee deleted (Masato Nagasawa)

#6 Updated by Kousuke Ebihara almost 8 years ago

  • Priority changed from Normal(通常) to High(高め)

#930 の変更を取り込んで以降のバージョン、およびそのバックポートによって後方互換性が壊れています。優先度を上げて取り組みたいです。

#7 Updated by Kousuke Ebihara almost 8 years ago

http://redmine.openpne.jp/issues/940#note-26 にてこのチケットも含めた現状の実装の問題について述べています。

#8 Updated by Minoru Takai almost 8 years ago

  • Status changed from New(新規) to Accepted(着手)
  • Assignee set to Minoru Takai

#940 と併せて担当します( #940 に絡む問題であるため takai が担当しますが、私よりも先にこのチケットを修正できそうであるという場合はコメントにその旨の記述をお願いします)。

#9 Updated by Minoru Takai almost 8 years ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

更新履歴 9072bbd231dc28faaf9b010c0622fe7c99639d89 で適用されました。

#10 Updated by Minoru Takai almost 8 years ago

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() を呼ぶようにしていますが、これがパフォーマンス的に好ましくなかったり、よりスマートな方法があればフィードバックを頂きたいです。

#11 Updated by Rimpei Ogawa almost 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

修正内容確認しました。問題はありませんでした。

advancedValidator() と比較して、sfValidatorInteger を利用した場合に clean() の結果を使用していない点が若干気になりましたが、intval() をしているかどうかの違いなのでこの後の処理には影響はないと判断しました。

また、validateValueMin(), validateValueMax() に分ける方法はとくに問題ないと思いました(よりスマートな方法を思いつきませんでした:無理に1つにまとめても例外処理が面倒だったりするのでこの方法でいいかなと思います)。

#12 Updated by Minoru Takai almost 8 years ago

  • Status changed from Pending Testing(テスト待ち) to Rejected(差し戻し)
  • % Done changed from 70 to 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() の結果を使用していない点」は気づいていませんでした。行ったほうが適切なようであれば見直しと同時にこれを追加しようと思います。

#13 Updated by Minoru Takai almost 8 years ago

  • Status changed from Rejected(差し戻し) to Accepted(着手)

#14 Updated by Rimpei Ogawa almost 8 years ago

Minoru Takai は書きました:

具体的には、日付型のバリデートに関して、このチケットで扱っている管理画面側で、以前は opValidatorDate を用いていましたが、今回の修正で sfValidatorDate を用いてしまっています。なお、メンバー側の日付入力部分では、以前も現在も opValidatorDate を用いています。これが異なっていることは適切ではないはずなので、 opValidatorDate を用いるように修正します。

これはレビュー側で見落としていました。修正方針問題ないと思います。

t930 修正前と修正後の差分 を改めて確認しました。 note-11 での指摘について、「advancedValidator() と比較して、sfValidatorInteger を利用した場合に clean() の結果を使用していない点」は気づいていませんでした。行ったほうが適切なようであれば見直しと同時にこれを追加しようと思います。

通常のフォームクラスを利用した処理では save() 時に利用される値はバリデーターの clean() の返り値となっており、今回の箇所に関しては opValidatorDate のみ特別にこの clean() の返り値を利用しないような変更をしているものだという認識です。

sfValidatorInteger の clean() 処理は単純で副作用も予測できる範囲なので問題なしとしましたが、他のフォームフィールドとの扱いを揃えるという意味では、特別な理由がない限りは clean() の結果を利用するようにした方がよいのではないかと思います。

#15 Updated by Shingo Yamada almost 8 years ago

  • 360対象 set to beta13

#16 Updated by Minoru Takai almost 8 years ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)

更新履歴 afee7267847f2350da4accd21e181a6c1729be3b で適用されました。

#17 Updated by Minoru Takai almost 8 years ago

更新履歴 a932c17831a732fee4754a3b4242d2fe36a3d35a で適用されました。

#18 Updated by Minoru Takai almost 8 years ago

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 で既にレビューされていますが、この修正内容と、改めて全体のレビューを再度お願いしたいと思います。

#19 Updated by Maki Takahashi almost 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)
  • a932c178
    • lib/form/doctrine/ProfileForm.class.phpの124行目 if文の記述がコーディング規約に反しています(終了の波括弧が改行されていない)
if (!($validator instanceof opValidatorDate)) {
  • afee7267
    • 追加された「本来あるべき仕様(相対的な日付が入力できる仕様)についてのメッセージ」の英文が不自然なようです
If you input "now," 〜(省略)

nowの後にあるカンマは"の外にくるのが正しいのではないでしょうか。

その他、動作などについては問題ないようです。

#20 Updated by Minoru Takai almost 8 years ago

  • Status changed from Rejected(差し戻し) to Pending Review(レビュー待ち)

更新履歴 b51d54adc43ef3dc945236b714e692b96f9da74e で適用されました。

#21 Updated by Minoru Takai almost 8 years ago

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 ...)

ついでに、メッセージについて日本語訳も少し修正しました。

#22 Updated by Maki Takahashi almost 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

b51d54ad 修正確認いたしました。ありがとうございます。

redmine上のapps/pc_backend/i18n/messages.ja.xmlのdiff表示が(safari5.1にて)文字化けしている部分がありますが
diff表示のみ文字化けしているだけのようです。

#23 Updated by Yuma Sakata over 7 years ago

  • Status changed from Pending Testing(テスト待ち) to Fixed(完了)
  • % Done changed from 70 to 100

テストOKです。

#24 Updated by kaoru n over 3 years ago

  • 3.8 で発生するか set to Unknown (未調査)

Also available in: Atom PDF