OpenPNE 3

再現確認¶

以下バージョンで再現確認できました。

• 3.6.1
• 3.4.18

修正内容の修正パッチリンク先が存在しなかったため別の方法で修正．
修正内容の二番目の実装の必要性がわからなかったため一番目の修正内容の実装のみを行った．

以下は以前チケットに書かれていた内容．

h3. Way to fix (修正内容)

* parse_url() の戻り値配列に含まれる port を保持。(lib/config/opApplicationConfiguration.class.php)
* sfPatternRoutingを継承したクラス opPatternRouting で修正。symfony側（sfRouting）には手をつけない。

修正パッチ: https://github.com/tozuka/OpenPNE3/commit/1da0e7306a7815a8502f7524a6b26f1436096bd7

h3. 備考

config/OpenPNE.yml.sample のように<pre>
base_url: "http://example.com" 
</pre>が指定されていると、実際のリクエストからURLをポート番号込みで読み取って絶対URLを生成してくれる実装になっているが、op_base_url までは書き換えてくれないのでメールのフッタには http://example.com と表示される。

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

sslを用いる場合に正常に動作しない可能性があるため，チケットのステータスを再度「Accepted」に戻します．

修正方法¶

note-5 で書き換える前の方針で修正を行った．
つまり，ベースにする URL は実際に生成するまで不明なため， opPatternRouting の generate() が呼び出されるときに決定するように修正を行った．

diff --git a/lib/config/opApplicationConfiguration.class.php b/lib/config/opApplicationConfiguration.class.php
index cb69ad2..e6158f3 100644
--- a/lib/config/opApplicationConfiguration.class.php
+++ b/lib/config/opApplicationConfiguration.class.php
@@ -582,35 +582,9 @@ abstract class opApplicationConfiguration extends sfApplicationConfiguration
     sfConfig::set('sf_app', $application);
     $configuration->setAppDir(sfConfig::get('sf_apps_dir').DIRECTORY_SEPARATOR.$application);

-    $settings = sfDefineEnvironmentConfigHandler::getConfiguration($configuration->getConfigPaths('config/settings.yml'));
-    $isNoScriptName = !empty($settings['.settings']['no_script_name']);
-
     $options = $context->getRouting()->getOptions();
-    $url = sfConfig::get('op_base_url');
-    if ('http://example.com' !== $url)
-    {
-      $parts = parse_url($url);
-
-      $parts['path'] = isset($parts['path']) ? $parts['path'] : '';
-      $options['context']['prefix'] =
-        $this->getAppScriptName($application, sfConfig::get('sf_environment'), $parts['path'], $isNoScriptName);
--      if (isset($parts['host']))
-      {
-        $options['context']['host'] = $parts['host'];
-        if (isset($parts['port']))
-        {
-          $options['context']['host'] .= ':'.$parts['port'];
-        }
-      }
-    }
-    else
-    {
-      $path = preg_replace('#/[^/]+\.php$#', '', $options['context']['prefix']);
-      $options['context']['prefix'] = $this->getAppScriptName($application, sfConfig::get('sf_environment'), $path, $isNoScriptName);
-    }

-    $routing = new sfPatternRouting($context->getEventDispatcher(), null, $options);
+    $routing = new opPatternRouting($context->getEventDispatcher(), null, $options);
     $routing->setRoutes($config->evaluate($configuration->getConfigPaths('config/routing.yml')));
     $context->getEventDispatcher()->notify(new sfEvent($routing, 'routing.load_configuration'));

diff --git a/lib/routing/opPatternRouting.class.php b/lib/routing/opPatternRouting.class.php
new file mode 100644
index 0000000..9f8ac7e
--- /dev/null
+++ b/lib/routing/opPatternRouting.class.php
@@ -0,0 +1,97 @@
+<?php
+
+/**
+ * This file is part of the OpenPNE package.
+ * (c) OpenPNE Project (http://www.openpne.jp/)
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file and the NOTICE file that were distributed with this source code.
+ */
+class opPatternRouting extends sfPatternRouting
+{
+
+  public function generate($name, $params = array(), $absolute = false)
+  {
+    $app = sfConfig::get('sf_app');
+    $sslAppRequiredList = sfConfig::get('op_ssl_required_applications', array());
+    $sslActionRequiredList = sfConfig::get('op_ssl_required_actions', array($app => array()));
+
+    if (sfConfig::get('op_use_ssl', false)
+      && in_array($app, $sslAppRequiredList)
+      && isset($params['module']) && isset($params['action'])
+      && in_array($params['module'].'/'.$params['action'], $sslActionRequiredList[$app])
+    )
+    {
+      $this->options['context']['is_secure'] = true;
+      $sslBaseUrls = sfConfig::get('op_ssl_base_url');
+      $url = $sslBaseUrls[$app];
+      $isDefault = 'https://example.com' === $url;
+    }
+    else
+    {
+      $this->options['context']['is_secure'] = false;
+      $url = sfConfig::get('op_base_url');
+      $isDefault = 'http://example.com' === $url;
+    }
+
+    $parts = parse_url($url);
+
+    $configuration = sfContext::getInstance()->getConfiguration();
+    $settings = sfDefineEnvironmentConfigHandler::getConfiguration($configuration->getConfigPaths('config/settings.yml'));
+    $isNoScriptName = !empty($settings['.settings']['no_script_name']);
+
+    if (!$isDefault)
+    {
+      $parts['path'] = isset($parts['path']) ? $parts['path'] : '';
+      $this->options['context']['prefix'] =
+        $this->getAppScriptName($app, sfConfig::get('sf_environment'), $parts['path'], $isNoScriptName);
+
+      if (isset($parts['host']))
+      {
+        $this->options['context']['host'] = $parts['host'];
+        if (isset($parts['port']))
+        {
+          $this->options['context']['host'] .= ':'.$parts['port'];
+        }
+      }
+    }
+    else
+    {
+      $path = preg_replace('#/[^/]+\.php$#', '', $this->options['context']['prefix']);
+      $this->options['context']['prefix'] = $this->getAppScriptName($app, sfConfig::get('sf_environment'), $path, $isNoScriptName);
+    }
+
+    return parent::generate($name, $params, $absolute);
+  }
+
+  // equals opApplicationConfiguration#getAppScriptName
+  private function getAppScriptName($application, $env, $prefix, $isNoScriptName = false)
+  {
+    if ($isNoScriptName)
+    {
+      return $prefix;
+    }
+
+    if ('/' === $prefix)
+    {
+      $prefix = '';
+    }
+
+    $name = $prefix.'/'.$application;
+    if ($env !== 'prod')
+    {
+      $name .= '_'.$env;
+    }
+    $name .= '.php';
+
+    return $name;
+  }
+
+}

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

ロジックが間違っていないか¶

note-15 でレビューが通っていますが、一点確認させてください。

@@ -586,8 +586,19 @@ abstract class opApplicationConfiguration extends sfApplicationConfiguration
     $isNoScriptName = !empty($settings['.settings']['no_script_name']);

     $options = $context->getRouting()->getOptions();
-    $url = sfConfig::get('op_base_url');
-    if ('http://example.com' !== $url)
+    if ($options['context']['is_secure'])
+    {
+      $sslBaseUrls = sfConfig::get('op_ssl_base_url');
+      $url = $sslBaseUrls[$application];
+      $isDefault = 'https://example.com' === $url;
+    }
+    else
+    {
+      $url = sfConfig::get('op_base_url');
+      $isDefault = 'http://example.com' === $url;
+    }
+
+    if ($isDefault)
     {
       $parts = parse_url($url);

もともとは

    $url = sfConfig::get('op_base_url');
    if ('http://example.com' !== $url)
    {
      $parts = parse_url($url);

と、 $url を取得し、それが example.com ではない場合に if 文に入っていますが、修正後は、

    if ($options['context']['is_secure'])
    {
      $sslBaseUrls = sfConfig::get('op_ssl_base_url');
      $url = $sslBaseUrls[$application];
      $isDefault = 'https://example.com' === $url;
    }
    else
    {
      $url = sfConfig::get('op_base_url');
      $isDefault = 'http://example.com' === $url;
    }

    if ($isDefault)
    {
      $parts = parse_url($url);

$url を取得し、それが example.com である場合（ $isDefault である場合）に if 文に入っています。これは意図した修正でしょうか。コミットログや note-14 からは、この条件式を反転させたことが妥当なのか評価できませんでした。

$isDefault 部分の保守性と可読性¶

補足程度ですが、 $isDefault への代入を各ブロックに記述するよりも外に出した方が保守性が高いように思います。更に、 $isDefault をこの if 文一箇所でしか使っていない（他所で使う予定がない）のであればこの変数は不要かもしれません。

    if ($options['context']['is_secure'])
    {
      $sslBaseUrls = sfConfig::get('op_ssl_base_url');
      $url = $sslBaseUrls[$application];
      $urlScheme = 'https://';
    }
    else
    {
      $url = sfConfig::get('op_base_url');
      $urlScheme = 'http://';
    }

    if ($urlScheme.'example.com' === $url) // ここは等号比較ではなく、否定等号比較か
    {
      $parts = parse_url($url);

• http://tools.ietf.org/html/rfc3986#page-16 （下記）あたりを見ても 'http://' までの文字列を表す適切な語が分かりませんでした。下記でいう scheme はより具体的には schemeName と表現できそうです。 schemePart はまた別の定義があるようなので scheme "://" を表すには適切ではなさそうです。ということで上記では $urlScheme という変数名を使ってみました。
  

        URI         = scheme ":" hier-part [ "?" query ] [ "#" fragment ]
  
        hier-part   = "//" authority path-abempty
                    / path-absolute
                    / path-rootless
                    / path-empty
  

さらに、ここまで書いて思いましたが、 URL が example.com かどうかを調べるだけであれば、 (http|https):// を用意して URL の完全一致比較をおこなうよりも、正規表現で書いたほうが可読性が高いかもしれません。

    if ($options['context']['is_secure'])
    {
      $sslBaseUrls = sfConfig::get('op_ssl_base_url');
      $url = $sslBaseUrls[$application];
    }
    else
    {
      $url = sfConfig::get('op_base_url');
    }

    if (preg_match('#^https?://example\.com$#', $url)) // ここは preg_match() ではなく、その否定 !preg_match() か
    {
      $parts = parse_url($url);

• ^$ ではなく \A\z を使うべきだという話がありますが、ここではその話は考慮していません。

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

確認しました。

``$isDefault`` の共通化の必要は現状ないと思いますし、可読性の面でも充分です（正規表現等でのマッチングに比べればこちらのアプローチのほうが優れています）。

テストOKです。