プロジェクト

全般

プロフィール

Bug(バグ) #2008

完了

config/OpenPNE.yml で80番以外のポート番号を含むURLをbase_urlに指定した場合、通知メール等に含まれるURLにポート番号が反映されない

Naoya Tozuka さんが13年以上前に追加. 約9年前に更新.

ステータス:
Fixed(完了)
優先度:
High(高め)
担当者:
対象バージョン:
開始日:
2011-04-19
期日:
進捗率:

100%

予定工数:
3.6 で発生するか:
Yes (はい)
3.8 で発生するか:
Unknown (未調査)

説明

Overview (現象)

config/OpenPNE.yml で80番以外のポート番号を含むURLをbase_urlに指定した場合、通知メール等に含まれるURLにポート番号が反映されない

Environment (再現バージョン)

OpenPNE3.6beta8

Way to repro (再現手順)

  1. config/OpenPNE.yml で80番以外のポート番号を含むURLをbase_urlに指定
  2. 招待メールを送ってみる
  3. 招待状本文中の「〜〜SNS に参加する」URLのリンクにポート番号が含まれない

例:

base_url: "http://sns.openpne.jp:8080" 
...
■ ××××SNS に参加する
http://sns.openpne.jp/member/register/token/xyxyxyxyxyxyxyxy
...

Causes (原因)

parse_url(sfConfig::get('op_base_url')) の戻り値配列に含まれる port が使われていない

Way to fix (修正内容)

  • parse_url() の戻り値配列に含まれる port を保持.
  • sfPatternRoutingを継承したクラス opPatternRouting を作成して修正.
  • ベースにする 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;
+  }
+
+}

備考

config/OpenPNE.yml.sample のように

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


関連するチケット 3 (0件未完了3件完了)

関連している OpenPNE 3 - Bug(バグ) #1675: 設定ファイルを記述し忘れた場合に、招待メールに記載されたURLにアクセスできないFixed(完了)Naoya Tozuka2010-10-14

操作
関連している OpenPNE 3 - Backport(バックポート) #2725: config/OpenPNE.yml で80番以外のポート番号を含むURLをbase_urlに指定した場合、通知メール等に含まれるURLにポート番号が反映されないFixed(完了)Yuya Watanabe2011-04-19

操作
関連している OpenPNE 3 - Backport(バックポート) #2726: config/OpenPNE.yml で80番以外のポート番号を含むURLをbase_urlに指定した場合、通知メール等に含まれるURLにポート番号が反映されないFixed(完了)Yuya Watanabe2011-04-19

操作

Mutsumi Imamura さんが13年以上前に更新

  • 対象バージョンOpenPNE 3.7.0 にセット

Yuma Sakata さんが約13年前に更新

  • 3.6 で発生するかYes (はい) にセット
  • 3.4 で発生するかYes (はい) にセット

再現確認

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

  • 3.6.1
  • 3.4.18

Yuma Sakata さんがほぼ13年前に更新

  • 優先度Normal(通常) から High(高め) に変更

Yuya Watanabe さんがほぼ13年前に更新

  • ステータスNew(新規) から Accepted(着手) に変更
  • 担当者Yuya Watanabe にセット

Yuya Watanabe さんがほぼ13年前に更新

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

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

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 と表示される。

Yuya Watanabe さんがほぼ13年前に更新

  • ステータスAccepted(着手) から Pending Review(レビュー待ち) に変更
  • 進捗率0 から 50 に変更

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

Yuya Watanabe さんがほぼ13年前に更新

  • ステータスPending Review(レビュー待ち) から Accepted(着手) に変更
  • 進捗率50 から 0 に変更

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

Yuya Watanabe さんがほぼ13年前に更新

修正方法

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;
+  }
+
+}

Yuya Watanabe さんがほぼ13年前に更新

Yuya Watanabe さんがほぼ13年前に更新

  • ステータスAccepted(着手) から Pending Review(レビュー待ち) に変更
  • 進捗率0 から 50 に変更

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

Yuya Watanabe さんがほぼ13年前に更新

Yuya Watanabe さんがほぼ13年前に更新

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

Yuya Watanabe さんがほぼ13年前に更新

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

Yuya Watanabe さんがほぼ13年前に更新

note-8 の内容は revert し, 現在のコンテキストが is_secure ならば ssl_base_url を見るように変更.

Kousuke Ebihara さんがほぼ13年前に更新

  • ステータスPending Review(レビュー待ち) から Pending Testing(テスト待ち) に変更
  • 進捗率50 から 70 に変更

Minoru Takai さんがほぼ13年前に更新

  • ステータスPending Testing(テスト待ち) から Rejected(差し戻し) に変更
  • 進捗率70 から 50 に変更

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

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 を使うべきだという話がありますが、ここではその話は考慮していません。

Yuya Watanabe さんがほぼ13年前に更新

問題

note-8 に記述されているコードを revert し,再度修正を行う際に lib/config/opApplicationConfiguration.class.php の 601 行目の $isDefault の部分を考慮することが抜けていました.その部分については以下のように修正します.

$isDefault の保守性についてですが,今後この設定値の部分が変化する可能性が多分にある場合はこの部分について何らかの手を打つべきではあると思います.
正規表現や部分的な共通化などの処理を行うことによって可読性や可用性が向上することが見込めますが,そのように幅をもたせるような記述をした場合に不具合の原因となる可能性が高くなってしまいます.
(例えば note-16 のような正規表現では base_url が https://example.com と書き換えられた場合でもデフォルト値として認識してしまいます.これが問題かどうかは今回とは別の議論となりますが.)
今回の場合では この部分は固定値の完全一致によってデフォルト値かどうかを判定することができ,今後変化することも現状考えられないと思います.
不具合の可能性を高めるようなリスクを負うほどの利点は得られないと考えて, $isDefault を正規表現や部分的な共通化などの対策については避ける方向で行きます.

修正内容

diff --git a/lib/config/opApplicationConfiguration.class.php b/lib/config/opApplicationConfiguration.class.php
index 5e32e01..09f653b 100644
--- a/lib/config/opApplicationConfiguration.class.php
+++ b/lib/config/opApplicationConfiguration.class.php
@@ -598,7 +598,7 @@ abstract class opApplicationConfiguration extends sfApplicationConfiguration
       $isDefault = 'http://example.com' === $url;
     }

-    if ($isDefault)
+    if (!$isDefault)
     {
       $parts = parse_url($url);

Yuya Watanabe さんがほぼ13年前に更新

  • ステータスRejected(差し戻し) から Accepted(着手) に変更
  • 進捗率50 から 0 に変更

Yuya Watanabe さんがほぼ13年前に更新

  • ステータスAccepted(着手) から Pending Review(レビュー待ち) に変更
  • 進捗率0 から 50 に変更

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

Kousuke Ebihara さんがほぼ13年前に更新

  • ステータスPending Review(レビュー待ち) から Pending Testing(テスト待ち) に変更
  • 進捗率50 から 70 に変更

確認しました。

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

Shouta Kashiwagi さんがほぼ13年前に更新

  • ステータスPending Testing(テスト待ち) から Fixed(完了) に変更
  • 進捗率70 から 100 に変更

テストOKです。

kaoru n さんが約9年前に更新

  • 3.8 で発生するかUnknown (未調査) にセット

他の形式にエクスポート: Atom PDF