Project

General

Profile

Bug(バグ) #1577

OpenPNE.ymlのbase_url にパスが含まれない場合に、デバッグモードでwarningメッセージが表示される場合がある

Added by Hidenori Goto almost 9 years ago. Updated about 8 years ago.

Status:
Fixed(完了)
Priority:
Normal(通常)
Target version:
Start date:
2010-09-10
Due date:
% Done:

100%

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

Description

症状

OpenPNE.yml の base_url に、 http://sns.example.com と末尾にスラッシュをつけずに設定した場合、デバッグモード実行していると warning が表示される箇所があります。より厳密には、末尾にスラッシュがない場合ではなく、URLにパスが含まれない場合です。

再現する箇所の例

環境

  • PHP 5.3.2 on Windows(XAMPP)
  • PHP 5.3.2 on Ubuntu 10.04

原因

opApplicationConfiguration::getAppRouting() 内で parse_url($url) の結果 $parts を使っている箇所があり、そこで $parts['path'] が定義されているかを確認せずに参照している。

http://sns.example.com のようなパスが含まれないURLの場合、 parse_url() が返す連想配列に path というキーは含まれない。

修正内容

  • lib/config/opApplicationConfiguration.class.php getAppRouting() メソッド内
    586-    $url = sfConfig::get('op_base_url');
    587-    if ('http://example.com' !== $url)
    588-    {
    589-      $parts = parse_url($url);
    590-      $options['context'] = array(
    591-        'prefix' => $this->getAppScriptName($application, sfConfig::get('sf_environment'), $parts['path'], $isNoScriptName),
    592-        'host'   => $parts['host'],
    593-      );
    594-    }
    

591 行目で $parts['path'] が存在する前提として参照しているため、この直前で $parts['path'] を明示的に定義する(存在しなければデフォルト値を与える)。

ここで与えるべきデフォルト値は null か空文字列がよいと思われるが、より適切な値があればそれを与える。

補足

$parts['path'] のデフォルト値については #1577:note-9 - #1577:note-10 の考察を以て「空文字列」とすることにした。

また、このチケットでは問題とされていないが、 $parts['host'] の値についても定義されていない場合を考慮するようにした。

更に、このチケットでの修正に併せて #1577:note-18 で言及されている問題も解消した(内容は #1577:note-16 を参照)。


Related issues

Related to OpenPNE 3 - Bug(バグ) #1632: [PATCH] opApplicationConfiguration::getAppScriptName()で、prefixの末尾スラッシュ除去処理が不十分 Won't fix(対応せず) 2010-10-01
Related to OpenPNE 3 - Backport(バックポート) #2163: OpenPNE.ymlのbase_url にパスが含まれない場合に、デバッグモードでwarningメッセージが表示される場合があ Fixed(完了) 2011-06-09
Related to OpenPNE 3 - Bug(バグ) #1155: There are wrong URL generating for op_base_url with path name (パス名付きの op_base_url に対して誤った URL 生成をしている箇所がある) Fixed(完了) 2010-06-08
Related to OpenPNE 3 - Bug(バグ) #1675: 設定ファイルを記述し忘れた場合に、招待メールに記載されたURLにアクセスできない Fixed(完了) 2010-10-14
Related to OpenPNE 3 - Backport(バックポート) #2325: OpenPNE.ymlのbase_url にパスが含まれない場合に、デバッグモードでwarningメッセージが表示される場合がある Fixed(完了) 2010-09-10 2011-10-06

Associated revisions

Revision 371b27cb (diff)
Added by Shogo Kawahara about 8 years ago

fixed opApplicationConfiguration::getAppRouting() because a warning occurred by it when the base_url's last character is not '/' (fixes #1577)

Revision 7a7e20ce (diff)
Added by Shogo Kawahara about 8 years ago

fixed opApplicationConfiguration::getAppRouting() correctly to get path information (fixes #1577)

History

#1 Updated by Kousuke Ebihara almost 9 years ago

  • 3.6 で発生するか set to Yes

#2 Updated by Kousuke Ebihara almost 9 years ago

  • Target version set to OpenPNE 3.6beta6

#3 Updated by Kousuke Ebihara almost 9 years ago

  • Target version changed from OpenPNE 3.6beta6 to OpenPNE 3.7.0

#4 Updated by Hidenori Goto almost 9 years ago

修正案に書いた修正では不完全なようでした。
もう少し調べてみます。

#5 Updated by Hidenori Goto almost 9 years ago

$parts['path']をチェックなしに利用していて警告がでることは確かです。
ですので、この部分は修正する必要があると思います。

後続のルーティング生成処理部分でも問題が見つかったため、別チケットにしてあります。

http://redmine.openpne.jp/issues/1632

#6 Updated by Shogo Kawahara about 8 years ago

  • Assignee set to Shogo Kawahara

#7 Updated by Shogo Kawahara about 8 years ago

  • Status changed from New(新規) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

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

#8 Updated by Kousuke Ebihara about 8 years ago

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

#9 Updated by Minoru Takai about 8 years ago

  • Status changed from Pending Testing(テスト待ち) to Rejected(差し戻し)
  • % Done changed from 70 to 50

以下の疑問について確認するため、一旦差し戻します。

疑問

今回の修正では parse_url() 直後に

      $parts['path'] = isset($parts['path']) ? $parts['path'] : '/';
      $parts['host'] = isset($parts['host']) ? $parts['host'] : '';

を追記していますが、この変更内容は問題ないものなのでしょうか。

  • base_url のURLにパスが含まれない場合に、 $parts['path'] が '/' であるものとしてしまってよいのでしょうか。
    • $parts['path'] を '' (空文字列)としておくよりも '/' である方がよいのでしょうか。
  • $parts['host'] に対しても同様の記述がありますが、この host に対する記述は必要なものなのでしょうか。もしこれが必要だとしたら、他にも併記すべき記述はないでしょうか。
  • この修正後に、 管理画面のヘッダ部にある(フロントエンドへの)リンクのURLが http://sns.example.com// のように末尾にスラッシュが連続(*1)してしまっていますが、これはこの修正に際して認識している挙動でしょうか( #1632 で関連問題が指摘されていますが、これ(*1)と同一の内容なのかは判断できていません)。

備考

この問題は、チケットで示されている通り opApplicationConfiguration::getAppRouting() メソッド内で parse_url() で得た連想配列に対して、定義されていない可能性のある 'path' インデックスの値を参照しているという問題です。

  • 修正前の lib/config/opApplicationConfiguration.class.php
        $url = sfConfig::get('op_base_url');
        if ('http://example.com' !== $url)
        {
          $parts = parse_url($url);
    
          $options['context'] = array(
            'prefix' => $this->getAppScriptName($application, sfConfig::get('sf_environment'), $parts['path'], $isNoScriptName),
            'host'   => $parts['host'],
          );
        }
    
  • A. URLがホスト名(直後にスラッシュなし)で終わる場合(パスなし)
    $ php -r 'var_dump(parse_url("http://example.com"));'
    
    array(2) {
      ["scheme"]=>  string(4) "http" 
      ["host"]=>  string(11) "example.com" 
    }
    
  • B. URLがホスト名(直後にスラッシュあり)で終わる場合(パスあり)
    $ php -r 'var_dump(parse_url("http://example.com/"));'
    
    array(3) {
      ["scheme"]=>  string(4) "http" 
      ["host"]=>  string(11) "example.com" 
      ["path"]=>  string(1) "/" 
    }
    
  • C. URLの末尾にスラッシュがないが、パスを含む場合(パスあり)
    $ php -r 'var_dump(parse_url("http://example.com/path/to"));'
    
    array(3) {
      ["scheme"]=>  string(4) "http" 
      ["host"]=>  string(11) "example.com" 
      ["path"]=>  string(8) "/path/to" 
    }
    

この問題が生じるのは「 base_url の末尾に '/' がない場合(AまたはC)」ではなく、「 base_url にパスが含まれない場合(A)」です。チケットタイトルは少し語弊があるかもしれません。

#10 Updated by Kousuke Ebihara about 8 years ago

ご指摘ありがとうございます。

  • $parts['path'] を '' (空文字列)としておくよりも '/' である方がよいのでしょうか。

opApplicationConfiguration::getAppScriptName() で $prefix が / である場合は空文字列として扱われます。ということで結果的には同じことになると思いましたが、パス名を暗黙的に / としてしまうのはまあ自然だろうと思って特に指摘しませんでした。

これだけ見ればどちらでも構わないと思います(が、後述する opApplicationConfiguration::getAppScriptName() のバグを考慮すると話が変わってきます)。

  • $parts['host'] に対しても同様の記述がありますが、この host に対する記述は必要なものなのでしょうか。

この後のルーティングコンテキストの作成において参照しているため必要なのだと思いましたが、 sfRouting::fixGeneratedUrl() を見ると空文字列を設定するのはかえってよくなさそうですね。

  • この修正後に、 管理画面のヘッダ部にある(フロントエンドへの)リンクのURLが http://sns.example.com// のように末尾にスラッシュが連続(*1)してしまっていますが、これはこの修正に際して認識している挙動でしょうか( #1632 で関連問題が指摘されていますが、これ(*1)と同一の内容なのかは判断できていません)。

この影響については認識していませんでした(ただ、この問題はこのチケットの修正が原因となって発生しているわけではありません)。修正がおこなわれているブロックを通らない場合(つまり、 op_base_url が http://example.com である場合)でも発生する問題であり、根本的な解決は #1632 のチケットで対応する必要があります。

ですが、このチケットの変更によって、いままで(偶然)生じなかったこの現象が発生するようになってしまうのは問題ですので、このチケットにおいては $parts['path'] は / ではなく空文字列にするのがよいのかなと思います。

まとめ

ということで、

  • $path['path'] は / ではなく空文字列にする
  • $path['host'] が存在しない場合は $options['context']['hosts'] を作らないようにする

ように修正する必要があると思います。

#11 Updated by Shogo Kawahara about 8 years ago

  • Status changed from Rejected(差し戻し) to Accepted(着手)

To: Takai, Ebihara

指摘把握です。
Takai さんが、 Ebihara さんの まとめ に納得したのであれば、それで修正させていただきます。

#12 Updated by Minoru Takai about 8 years ago

(note-11) 着手ありがとうございます(担当しようとかと思っていました)。 note-10 のまとめで示されている修正方針は良いと思っています。

(note-15 の修正を見て note-10 の記述と異なっていたのが気になったため、確認の意味も含めて追記)

  • $path['host'] が存在しない場合は $options['context']['hosts'] を作らないようにする

$options の配列のインデックスは(hosts ではなく) $options['context']['host'] ですね。

#13 Updated by Shogo Kawahara about 8 years ago

(note-12) ラジャー

#14 Updated by Shogo Kawahara about 8 years ago

  • Subject changed from OpenPNE.ymlのbase_urlで末尾に「/」がないURLを設定した場合に、デバッグモードでwarningメッセージが表示される場合がある to OpenPNE.ymlのbase_url にパスが含まれない場合に、デバッグモードでwarningメッセージが表示される場合がある

#15 Updated by Shogo Kawahara about 8 years ago

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

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

#16 Updated by Minoru Takai about 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)

note-15 をレビューしました。一点疑問があります。

note-15 の修正前と修正後の違い

  • $options['context'] の値について: note-15 修正前
    -      $options['context'] = array(
    -        'prefix' => $this->getAppScriptName($application, sfConfig::get('sf_environment'), $parts['path'], $isNoScriptName),
    -        'host'   => $parts['host'],
    -      );
    
    • 修正前は、このブロック内で $options['context'] = array() という形で代入されているので $options['context'] に元々あった値は上書きされており、(このコードから明らかですが) $options['context'] には prefix と host というキーしかありません。
      • 手元の環境で確認した、このブロック直後での $options['context'] の値( sns.example.com の部分は書き換えました)
        array
          'prefix' => string '/' (length=1)
          'host' => string 'sns.example.com' (length=15)
        
  • $options['context'] の値について: note-15 修正後
    +      $options['context']['prefix'] =
    +        $this->getAppScriptName($application, sfConfig::get('sf_environment'), $parts['path'], $isNoScriptName);
    
    +      if (isset($parts['host']))
    +      {
    +        $options['context']['host'] = $parts['host'];
    +      }
    
    • 修正後は $options['context']['prefix'] を追加する記述から始まっており、$options['context'] に元々あった値が残っています。
      • 手元の環境で確認した、このブロック直後での $options['context'] の値( sns.example.com の部分は書き換えました。また、この確認は管理画面 pc_backend.php で行いました)
        array
          'path_info' => string '/' (length=1)
          'prefix' => string '' (length=0)
          'method' => string 'GET' (length=3)
          'format' => null
          'host' => string 'sns.example.com' (length=15)
          'is_secure' => boolean false
          'request_uri' => string 'http://sns.example.com/pc_backend.php' (length=37)
        

疑問

  • $options['context'] に含まれている値を残してしまっていることは問題ないでしょうか。
  • あるいは逆に、修正前の記述で(もともとの値を)上書きしてしまっていることは問題であると考えられるでしょうか。

この疑問は、補足で示している #1155 のコミット 59527e56 に対して言えるものかもしれません。

補足

以下は getAppRouting() が追記された #1155 のコミット 59527e56 でのコードですが、この時点で if 文を通らない場合は $options['context'] に含まれている値が残っていたということになります。つまり、このチケットでの修正前のコードの時点で以下 532 行目の if 文を通るか否かで $options['context'] の値を上書きするか否かが異なっていたということです。

530-    $options = $context->getRouting()->getOptions();
531-    $url = sfConfig::get('op_base_url');
532-    if ('http://example.com' !== $url)
533-    {
534-      $parts = parse_url($url);
535:      $options['context'] = array(
536-        'prefix' => $this->getAppScriptName($application, sfConfig::get('sf_environment'), $parts['path'], $isNoScriptName),
537-        'host'   => $parts['host'],
538-      );
539-    }
540-    else
541-    {
542:      $path = dirname($options['context']['prefix']);
543:      $options['context']['prefix'] = $this->getAppScriptName($application, sfConfig::get('sf_environment'), $path, $isNoScriptName);
544-    }

$options['context'] の元々の値を上書きしても残していてもどちらでもよいというのであれば note-15 の修正は問題ないと思います。

#17 Updated by Shogo Kawahara about 8 years ago

  • Status changed from Rejected(差し戻し) to Accepted(着手)

(note-15) これは見落としでした。

修正コード中での $options['context'] を利用する sfPatternRouting での挙動も考えつつ修正案出します。

#18 Updated by Shogo Kawahara about 8 years ago

(note-16) の件調査しました。

結論から言えば、 (note-15) の修正のほうが正しいのではないかと考えています。

修正中にある $options['context'] は sfPatternRouting のインスタンス生成時に
利用されます。このクラス中渡したパラメタを sfPatternRouting::fixGeneratedUrl() (sfRouting から継承)
で、利用します。

ここでは、 $options['context']['prefix'], $options['context']['host'], $options['context']['is_secure']
が利用されています。

このメソッドでは、 生成されたURL に $options['context']['prefix'] の値を前に追加し、
絶対パスを生成する場合で、$options['context']['host'] が存在する場合には
さらに http://host を前に追加します。
ただし、 $options['context']['is_secure'] が true の場合は https://host を前に追加します。

今までは、他アプリケーションの絶対URLを生成するときに、現URLが https://〜 の場合 http://〜
が生成される挙動でしたが、 この修正により 現URLが https://〜 の場合 https://〜 が生成されるようになります。
私は、後者の挙動が正しいのではないかと考えています。

#19 Updated by Shogo Kawahara about 8 years ago

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

#20 Updated by Minoru Takai about 8 years ago

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

(note-17 - note-19) 確認ありがとうございます。 note-18 の考察内容を以て note-16 の疑問が晴れるため、 note-15 の修正はOKとします。(万一、 note-18 の内容が誤っていたとしても、その誤りが発覚した時点で新たに問題提起をして解決することを予定します。)

ここまでの修正を以て、このチケットの内容には対応できたと判断します(コーディング規約の観点からも、特に問題はないと思うのでOKとします)。

#21 Updated by Minoru Takai about 8 years ago

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

動作テスト

この問題は、ソースコードの修正さえ見れば改善されているか否かは明らかであり、動作テストの重点はコードチェックに置かれます。ソースコードの修正は妥当だと判断できており、このチケットへの対応はOKと判断できます。

ついでにですが、 http://sns.example.com/pc_backend_dev.php のようなURLとなるSNS(テスト環境)において、修正前のコードでは「未定義の $parts['path'] を参照している旨の warning が表示されていること」を確認し、修正後のコードではこの警告が表示されなくなったことを確認しました。

完了とします。

Also available in: Atom PDF