OpenPNE 3

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

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

Minoru Takai は書きました:

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

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

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

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

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

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

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

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

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