Request::hasSession()の現在の実装の意図
- (意図A)クライアントから送られてきた(本来の意味の)リクエストがセッション有効状態か(=Cookieを持っているか)を調べる
- (意図B)単にセッションオブジェクトがあるかないかという判定
- (意図C)セッションオブジェクトがあり、開始済みかどうか
現在のhasSession()メソッドは、意図Aの実装であると考えられる。ただし、以下に挙げる呼び出し箇所では、意図Bなどが混在して用いられていると思われる。
Request::hasSession()の使われ方
- a. RequestListener L70 上記の意図で判定に用いている。
- b. RequestDataCollector L67 メソッドの意図と異なる使われ方と思われる。null !== $request->getSession() が適切ではないか。
- c. SecurityComponent Http/Firewall/ContextListener L65 メソッドの意図と異なる使われ方と思われる。null !== $request->getSession() が適切ではないか。
- d. SecurityComponent Http/Firewall/ExceptionListener L154 用いている意図がやや不明確。
Created
May 8, 2011 00:45
-
-
Save hidenorigoto/960995 to your computer and use it in GitHub Desktop.
Request::hasSession()について
RequestListenerへの修正の適用の件は、私の勘違いでしたね。すみませんでした、理解しました。
現時点では利用されている箇所も限られているので、hasSession()を廃止候補にした上で、書かれているコード例のように判定を各箇所で展開するという提案がよさそうですね。
修正してみました。以前のブランチはいったん破棄して、新たに変更を行っています(そのためSession::isStarted()は実装していません)。
https://github.com/fivestar/symfony/commits/session
fivestar/symfony@ae0ef9e
合わせてExceptionListenerで行っていた箇所を、EntryPointへ移動しました。
既に修正パッチがあがっているようでした。EntryPointへ移動した部分は、Johannes曰く、話し合いの結果ExceptionListenerから動かさないことを決めたとのことです。
あがっていたパッチではhasSession()はセッションオブジェクトの有無を確かめるメソッドに変更されており、CookieがあるかどうかをhasPreviousSession()として定義していました。
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
コメントには確かにそう書いてあるんですよね。その意図で判定を行うのであれば、hasSessionはなんの解決にもなっていないですよね。。。そう考えると、このパスを保存するロジックはここではなく個別のEntryPointに実装するのが適切にも思えてきました。
この部分は、RequestListenerでの判定箇所(意図A)の判定ロジックの部分という意味でした。Sessionの初期化を担っているのがRequestではなくRequestListenerであるため、初期化用のロジックとしてはRequestListenerにあるべき、という意味でした。そのためRequestListener側で状態を保持するという意図はありませんでした。
下記がRequestListenerの該当部分で、cookieの有無を判定する処理を展開したものです。この判定はRequestListener専用といってもよいのではないかと思うので、RequestListenerにあればよいという判断です。(この判定にたいするメソッド名という意味で、あげた3つはそれの候補でした)
hasSession()は意図がわかりづらいですよね。確かに廃止してしまって利用者側が適切に判定するほうが、混乱が起こりづらいと思います。