Bug(バグ) #2008
完了config/OpenPNE.yml で80番以外のポート番号を含むURLをbase_urlに指定した場合、通知メール等に含まれるURLにポート番号が反映されない
100%
説明
Overview (現象)¶
config/OpenPNE.yml で80番以外のポート番号を含むURLをbase_urlに指定した場合、通知メール等に含まれるURLにポート番号が反映されない
Environment (再現バージョン)¶
OpenPNE3.6beta8
Way to repro (再現手順)¶
- config/OpenPNE.yml で80番以外のポート番号を含むURLをbase_urlに指定
- 招待メールを送ってみる
- 招待状本文中の「〜〜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 と表示される。
Yuma Sakata さんが約13年前に更新
- 3.6 で発生するか を Yes (はい) にセット
- 3.4 で発生するか を Yes (はい) にセット
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年前に更新
- ステータス を Accepted(着手) から Pending Review(レビュー待ち) に変更
- 進捗率 を 0 から 50 に変更
更新履歴 9be543bd53d57c94a545968057441e5ff13c6d43 で適用されました。
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です。