OpenPNE 3

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

各プラグインも修正しました。
それぞれのプラグインリポジトリにpull requestしてます。
https://github.com/openpne-ospt/opAlbumPlugin/pull/10
https://github.com/tejimaya/opCommunityTopicPlugin/pull/1
https://github.com/balibali/opDiaryPlugin/pull/12
https://github.com/openpne-ospt/opOpenSocialPlugin/pull/22
https://github.com/ebihara/opWebAPIPlugin/pull/6

この修正を見て、より利便性のよい修正が可能ではないかと思ったのでコメントします。

LIKE 検索を行う側で、

$q->andWhere($column.' LIKE ?', '%'.Doctrine_Manager::connection()->formatter->escapePattern($value).'%');

のようにメソッドをコールするような修正となっていますが、コアにしてもプラグインにしても、LIKE検索を行う箇所で、わざわざこの長い記述を行なわせることは好ましくないのではないかと思いました。

opToolkit クラスなどに以下のように汎用的なメソッドを定義しておき、

diff --git a/lib/util/opToolkit.class.php b/lib/util/opToolkit.class.php
index d0db038..e5e0fe7 100644
--- a/lib/util/opToolkit.class.php
+++ b/lib/util/opToolkit.class.php
@@ -17,6 +17,16 @@
  */
 class opToolkit
 {
+  /**
+   * Returns the escaped string for a LIKE operator of DQL.
+   *
+   * @return string
+   */
+  public static function likeQuery($value)
+  {
+    return '%'.Doctrine_Manager::connection()->formatter->escapePattern($value).'%';
+  }
+
  /**
   * Returns the list of mobile e-mail address domains.
   *

LIKE 検索を行う側では以下のようにメソッドを呼ばせるようにするのはどうでしょうか。

$q->andWhere($column.' LIKE ?', opToolkit::likeQuery($value));

• likeQuery というメソッド名が適切かどうかは分かりませんが、 get*** や escape*** のようなメソッド名は似合わないように感じたので暫定的に likeQuery としています。
• opToolkit::likeQuery() は将来的に、引数を付けることで呼び出し元からエスケープ対象文字を変更したり、あるいは部分一致検索ではなく前方一致検索・後方一致検索を可能にしたりする拡張が可能かもしれません。
• 余談ですが、 opToolkit クラス内の /** から始まる既存のコメントは何故かインデントがずれているようです。

また、プラグイン側での修正を見ましたが、

      if (defined('OPENPNE_VERSION') && version_compare(OPENPNE_VERSION, '3.6beta13-dev', '>='))
      {
        $keyword = Doctrine_Manager::connection()->formatter->escapePattern($keyword);
      }

のような記述が各所に追記されていますが、同じ目的で記述しているのであれば、なるべく記述を抽象化した方が保守しやすいのではないかと思います。各所に version_compare(OPENPNE_VERSION, '3.6beta13-dev', '>=') のような記述が書かれているのが好ましくないように思います。

likeQuery() を

  public static function likeQuery($value)
  {
    if (defined('OPENPNE_VERSION') && version_compare(OPENPNE_VERSION, '3.6beta13-dev', '>='))
    {
      $value = Doctrine_Manager::connection()->formatter->escapePattern($value);
    }

    return '%'.$value.'%';
  }

のように定義し、これを各所でコールした方が、後に本件の仕様を変更したい場合にも、このメソッドを書き換えるだけで済むため好ましいように思います。

実装不備ではありませんが、提案について確認していただきたいのでステータスを差し戻しに変更します。

プラグイン側の実装も鑑みて、現状のままでも良いのではないかと判断されるのであれば、その旨をコメントして改めてレビュー待ちにしていただければと思います。

8/5(金)追記¶

このコメント内容について実装者と検討を行いました。検討内容とは別ですが、上記のコメントに若干誤りがあるので補足しておきます。

likeQuery() を

  public static function likeQuery($value)
  {
    if (defined('OPENPNE_VERSION') && version_compare(OPENPNE_VERSION, '3.6beta13-dev', '>='))
    {
      $value = Doctrine_Manager::connection()->formatter->escapePattern($value);
    }

    return '%'.$value.'%';
  }

のように定義し、これを各所でコールした方が、
...

$q->andWhere($column.' LIKE ?', opToolkit::likeQuery($value));

と示しましたが、各所でいきなり likeQuery() をコールしても、このメソッドが定義されるのはコアのマイナーバージョンが最新版のもののみなので、新しいプラグインと古いコアを使う場合に未定義のメソッドを呼ぶことによる問題が生じます。

後半の内容については以下のようにすることになるかもしれません。

• メソッドの定義
  

    public static function likeQuery($value)
    {
      return '%'.Doctrine_Manager::connection()->formatter->escapePattern($value).'%';
    }
  

  • 実際に内部でどのようにエスケープをするか（Doctrine が用意している escapePattern() を用いるか、あるいは使うにしても Doctrine_Manager::connection()->formatter 以外を経由して escapePattern() を用いるなど）を変更したい時は、このメソッドの実装を変更することで対応できるようにする。
  • このメソッドで % を付加する目的は、LIKE検索を行いたい箇所で、「部分一致検索のためには前後に % を付加する」という事実を隠蔽（抽象化）するためです。LIKE検索（部分一致検索）したいときは、とにかく opToolkit::likeQuery() に値を渡して、返り値を andWhere() などに渡せばよいようにするという発想に基づいています。
• 呼び出し側の記述例(1)
  

  $q->andWhere($column.' LIKE ?', method_exists('opToolkit', 'likeQuery') ? opToolkit::likeQuery($value) : '%'.$value.'%');
  

  • この例では likeQuery() が 検索文字列 $value の前後に（部分一致検索とするためのワイルドカード） % を付加した文字列を返す仕様を前提としています。
• 呼び出し側の記述例(2)
  

  if (method_exists('opToolkit', 'likeQuery'))
  {
    $value = opToolkit::likeQuery($value);
  }
  $q->andWhere($column.' LIKE ?', '%'.$value.'%');
  

  • この(2)の例では、 likeQuery() が '%'.(検索文字列).'%' を返すようにしていても、部分位置検索を前提としていれば andWhere() の第2引数の時点で '%'.('%'.(検索文字列).'%').'%' となるだけで、検索結果には影響は無いと思います（ % が何個連続してもワイルドカードとしての意味は変わらない）。

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

opDoctrineQuery クラスにメソッドを追加して対応する方針としたようですが、チケット作成時の問題に対する対応としてこの方針での修正を適用することはよいと思います。 note-15 のような話もあるようですが、これまでワイルドカードに対して何の考慮もしていなかったことに対して、少なくとも MySQL を使用した環境でこれが対応されていればよく、他のDBでも対応したいとなればこのチケットでの改善を基に（別チケットで）対応すればよいと考えています。

指摘点¶

note-12 から note-14 での修正内容は概ねよいと思っています。細かい点ですが以下について確認お願いします。

• (1) {where|andWhere}Like($column, $value) への書き換えの対応漏れ
  • コア側で 'LIKE' が使われている箇所（ plugins 下のコアに含まれるプラグインは該当なし）
  • (1-1) FileTable クラス内で直接 LIKE 文が使われている箇所は、書き換える必要はないでしょうか。
  • (1-2) opFormItemGenerator クラス内は書き換えるべきだと思いますが、書き換える必要はないでしょうか。
    

    $ ack 'LIKE' -w --ignore-dir=vendor lib apps -C1
    
    lib/model/doctrine/FileTable.class.php
    46-    $q = $this->getImageOrderdQuery()
    47:      ->where('type NOT LIKE ?', 'image%');
    48-    return $this->getPager($q, $page, $this->getValidPagerSize($size));
    --
    53-    $q = $this->getImageOrderdQuery()
    54:      ->where('type LIKE ?', 'image%');
    55-    return $this->getPager($q, $page, $this->getValidPagerSize($size));
    
    lib/util/opFormItemGenerator.class.php
    364-      case 'date':
    365:        $q->andWhere($column.' LIKE ?', $value);
    366-        break;
    
    lib/util/opDoctrineQuery.class.php
    316-    {
    317:      $this->andWhere($expr.' NOT LIKE ?', $match);
    318-    }
    --
    320-    {
    321:      $this->andWhere($expr.' LIKE ?', $match);
    322-    }
    

• (2) MATCH_BROAD 定数の順序と値
  • opDoctrineQuery では以下のように記述されていますが、定数の記述順序と、 case 文の記述順序が一致していないのが気になります。
    

    23:  const MATCH_EXACT = 0;
    :
    28:  const MATCH_BROAD = 1;
    :
    33:  const MATCH_LEFT = 2;
    :
    38:  const MATCH_RIGHT = 3;
    --
    298-  public function andWhereLike($expr, $param, $matchType = self::MATCH_BROAD, $not = false)
    299-  {
    300-    $match = $this->escapePattern($param);
    301-
    302-    switch ($matchType)
    303-    {
    304-      case self::MATCH_LEFT:
    305-        $match = $match.'%';
    306-        break;
    307-      case self::MATCH_RIGHT:
    308-        $match = '%'.$match;
    309-        break;
    310-      case self::MATCH_BROAD:
    311-        $match = '%'.$match.'%';
    312-        break;
    313-    }
    

  • 順序の不一致を直すのに加え、改善が可能かもしれません。例えば $matchType = opDoctrineQuery::MATCH_LEFT & opDoctrineQuery::MATCH_RIGHT; とした場合に結果として opDoctrineQuery::MATCH_BROAD となるように値を設定すると、この定数の利便性が上がります。
    • 訂正： opDoctrineQuery::MATCH_LEFT & opDoctrineQuery::MATCH_RIGHT と書きましたが、 & ではなく | ですね。
  • 修正案
    

    const MATCH_EXACT = 0;
    const MATCH_BROAD = 1;
    const MATCH_LEFT = 2;
    const MATCH_RIGHT = 3;
    
    これを以下のように書き換えます。
    
    const MATCH_EXACT = 0;
    const MATCH_LEFT = 1;
    const MATCH_RIGHT = 2;
    const MATCH_BROAD = 3;
    

• (3) opDoctrineQuery::escapePattern() がコーディング規約に違反しています
  • 既存のコードをコピーペーストしたのかもしれませんが、インデント等が一部不適切です。
  • (3-1) 333 行目：インデントが多いです
  • (3-2) 340 行目：インデントが多いです
  • (3-3) 340 行目：文字列結合演算子の前後にスペースがあります
    

    327:  public function escapePattern($text)
    328-  {
    329-    $conn = $this->getConnection();
    330-
    331:    if (!$conn->string_quoting['escape_pattern'])
    332-    {
    333-        return $text;
    334-    }
    335-    $tmp = $conn->string_quoting;
    336-
    337:    $text = str_replace($tmp['escape_pattern'], $tmp['escape_pattern'].$tmp['escape_pattern'], $text);
    338-
    339-    foreach ($conn->wildcards as $wildcard) {
    340:        $text = str_replace($wildcard, $tmp['escape_pattern'] . $wildcard, $text);
    341-    }
    342-
    343-    return $text;
    344-  }
    

ちなみに、 matchType の定数名が **LEFT, **RIGHT, **BROAD でよいかなども考えましたがこれはこれで悪くないと思いました。上記の指摘項目以外には問題はないと思います。

以上について、確認した上で修正する必要がない箇所があればそのままでもよいと思います。修正すべきであればコミットをお願いします。

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

note-19 をレビューしました。レビュー OK です。

• lib/model/doctrine/FileTable.class.php と lib/util/opFormItemGenerator.class.php を修正していない点については、今回の修正での変更箇所を最小限にするという意図があるとのことなので、このままで OK とします。
• note-16 での他の指摘内容が修正されており、コア側については note-19 までの修正で「意図しないワイルドカードを含んだ検索がされてしまう問題」が解消できていると思われるので OK とします。

このチケットでの対応¶

LIKE 検索のためのメソッドを用意する¶

• andWhere(opToolkit::likeQuery($value)) のようなラッパではなく、 opDoctrineQuery に andWhereLike() のようなメソッドを直接追加し、それを用いるようにした。
• 今回は、検索で問題が生じている箇所に対応できればよいため、必要最低限なメソッドしか追加していない。
  • whereLike() と andWhereLike() しか追加しておらず、 addWhereLike(), orWhereLike(), whereNotLike() などは定義していない。
  • 用意したほうがよい状況になった場合に追加する可能性があるが、今回のチケットではバグ対応に重点をおき、メソッドの追加は扱わないこととする。
• andWhereLike() の実装についても必要最低限にすべきかもしれないが、必ずエスケープを行う部分一致検索（MATCH_BROAD）の場合のみの実装だと、部分一致以外の検索をしたい場合や、呼び出し側の値が意図してワイルドカードを値に含めることができず、 andWhereLike() というメソッドを使える場面が限定されてしまうことを懸念して、ある程度柔軟な実装にしておくこととした。

用意したメソッドを使うように書き換える¶

コア側に含まれる「意図しないワイルドカードを含んだ検索がされてしまう問題」が生じてしまう箇所のみを修正することとした。そのため lib/model/doctrine/FileTable.class.php と lib/util/opFormItemGenerator.class.php は意図的に修正しないままにしている。このチケットでは修正しないままにしているだけであり、修正しないことが好ましいという意味ではない。

テストOKです