Project

General

Profile

Bug(バグ) #2008

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

Added by Naoya Tozuka about 8 years ago. Updated over 3 years ago.

Status:
Fixed(完了)
Priority:
High(高め)
Assignee:
Target version:
Start date:
2011-04-19
Due date:
% Done:

100%

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

Description

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


Related issues

Related to OpenPNE 3 - Bug(バグ) #1675: 設定ファイルを記述し忘れた場合に、招待メールに記載されたURLにアクセスできない Fixed(完了) 2010-10-14
Related to OpenPNE 3 - Backport(バックポート) #2725: config/OpenPNE.yml で80番以外のポート番号を含むURLをbase_urlに指定した場合、通知メール等に含まれるURLにポート番号が反映されない Fixed(完了) 2011-04-19
Related to OpenPNE 3 - Backport(バックポート) #2726: config/OpenPNE.yml で80番以外のポート番号を含むURLをbase_urlに指定した場合、通知メール等に含まれるURLにポート番号が反映されない Fixed(完了) 2011-04-19

Associated revisions

Revision bdca8737 (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2008) add port to generated url

Revision 9be543bd (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2008) fixed to generate url considering SSL and the port

Revision 4baf7971 (diff)
Added by Yuya Watanabe over 7 years ago

Revert "(fixes #2008) fixed to generate url considering SSL and the port"

This reverts commit 9be543bd53d57c94a545968057441e5ff13c6d43.

Revision d65df79b (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2008) fixed to generate url considering SSL and the port

Revision 2ed9c936 (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2008) fixed to flip default flag to evaluate correct boolean

Revision 013554b4 (diff)
Added by Yuya Watanabe about 7 years ago

(fixes #2008) add port to generated url

Revision 238d4c94 (diff)
Added by Yuya Watanabe about 7 years ago

(fixes #2008) fixed to generate url considering SSL and the port

Revision 2a845267 (diff)
Added by Yuya Watanabe about 7 years ago

Revert "(fixes #2008) fixed to generate url considering SSL and the port"

This reverts commit 9be543bd53d57c94a545968057441e5ff13c6d43.

Revision f7cc71d8 (diff)
Added by Yuya Watanabe about 7 years ago

(fixes #2008) fixed to generate url considering SSL and the port

Revision b41289b8 (diff)
Added by Yuya Watanabe about 7 years ago

(fixes #2008) fixed to flip default flag to evaluate correct boolean

History

#1 Updated by Mutsumi Imamura almost 8 years ago

  • Target version set to OpenPNE 3.7.0

#2 Updated by Yuma Sakata over 7 years ago

  • 3.6 で発生するか set to Yes (はい)
  • 3.4 で発生するか set to Yes (はい)

再現確認

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

  • 3.6.1
  • 3.4.18

#3 Updated by Yuma Sakata over 7 years ago

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

#4 Updated by Yuya Watanabe over 7 years ago

  • Status changed from New(新規) to Accepted(着手)
  • Assignee set to Yuya Watanabe

#5 Updated by Yuya Watanabe over 7 years ago

  • Description updated (diff)

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

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

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

#6 Updated by Yuya Watanabe over 7 years ago

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

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

#7 Updated by Yuya Watanabe over 7 years ago

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

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

#8 Updated by Yuya Watanabe over 7 years ago

  • Description updated (diff)

修正方法

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

#9 Updated by Yuya Watanabe over 7 years ago

  • Description updated (diff)

#10 Updated by Yuya Watanabe over 7 years ago

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

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

#11 Updated by Yuya Watanabe over 7 years ago

  • Description updated (diff)

#12 Updated by Yuya Watanabe over 7 years ago

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

#13 Updated by Yuya Watanabe over 7 years ago

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

#14 Updated by Yuya Watanabe over 7 years ago

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

#15 Updated by Kousuke Ebihara over 7 years ago

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

#16 Updated by Minoru Takai over 7 years ago

  • Status changed from Pending Testing(テスト待ち) to Rejected(差し戻し)
  • % Done changed from 70 to 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 を使うべきだという話がありますが、ここではその話は考慮していません。

#17 Updated by Yuya Watanabe over 7 years ago

問題

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

#18 Updated by Yuya Watanabe over 7 years ago

  • Status changed from Rejected(差し戻し) to Accepted(着手)
  • % Done changed from 50 to 0

#19 Updated by Yuya Watanabe over 7 years ago

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

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

#20 Updated by Kousuke Ebihara over 7 years ago

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

確認しました。

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

#21 Updated by Shouta Kashiwagi over 7 years ago

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

テストOKです。

#22 Updated by kaoru n over 3 years ago

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

Also available in: Atom PDF