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()について
各箇所での利用意図の認識はほぼ同じです。
d の箇所については、コメントが「basic認証の場合はセッションを使っていなくても動作する」というような意味にとれました。この場合、この時点でセッションが開始していなくても、セッション機能が有効になっていれば無理やりセッションを開始させる（意図Bの判定）のか、セッションが開始していないなら戻りURLを設定しない（意図Cの判定）なのかは、読み取れませんでした。
対応についてですが、RequestListenerに判定メソッドを追加するのは適切ではないと思います
- （現時点では）RequestListenerはFrameworkBundle内のクラスなので、Component側から依存すべきではないと思います。
- RequestListenerはRequestに対するサービスであって状態を保持するものではないという認識です。
どのクラスにどのメソッドを実装すべきか
- hasSessionCookie → Request
- isSessionReady （Sessionがstartしているかどうか？） → Sessionに実装（isStarted?）し、Requestにプロキシメソッド
Request::hasSession() は分かりづらいので廃止を提案した方がよさそうですよね。
d の箇所については、コメントが「basic認証の場合はセッションを使っていなくても動作する」というような意味にとれました。この場合、この時点でセッションが開始していなくても、セッション機能が有効になっていれば無理やりセッションを開始させる（意図Bの判定）のか、セッションが開始していないなら戻りURLを設定しない（意図Cの判定）なのかは、読み取れませんでした。
コメントには確かにそう書いてあるんですよね。その意図で判定を行うのであれば、hasSessionはなんの解決にもなっていないですよね。。。そう考えると、このパスを保存するロジックはここではなく個別のEntryPointに実装するのが適切にも思えてきました。
対応についてですが、RequestListenerに判定メソッドを追加するのは適切ではないと思います
この部分は、RequestListenerでの判定箇所(意図A)の判定ロジックの部分という意味でした。Sessionの初期化を担っているのがRequestではなくRequestListenerであるため、初期化用のロジックとしてはRequestListenerにあるべき、という意味でした。そのためRequestListener側で状態を保持するという意図はありませんでした。
下記がRequestListenerの該当部分で、cookieの有無を判定する処理を展開したものです。この判定はRequestListener専用といってもよいのではないかと思うので、RequestListenerにあればよいという判断です。（この判定にたいするメソッド名という意味で、あげた3つはそれの候補でした）
class RequestListener
{
protected function initializeSession(Request $request, $master)
{
// ...
// starts the session if a session cookie already exists in the request...
if (null !== $request->getSession() && $request->cookies->has(session_name())) {
$request->getSession()->start();
}
}
}
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
下記は、メソッドの使われ方を上からそれぞれa,b,c,dとした場合、意図A,B,Cのどれが当てはまるかについての僕の意見です。副作用という言葉を用いていますが、これは不要な
session_start()の呼び出しにより空のセッションファイルやレコードが生成されることを指しています。auto_startの状態に問わずセッションを開始してよいと思われる（副作用がないと見なされるため）Session::getAttributes()は内部で自動的にセッションを開始しないので、Sessionオブジェクトがあれば結果は変わらないSession::set()を呼び出しているので、自動的にセッションが開始される副作用がある推測ですが、おそらくこのメソッドはaのために用意されたのが始まりだと思われます。これはセッションを開始してよいかどうかを判定するためのもので、おそらくこの意図(A)で利用するケースはここだけでしょう。
それ以外の箇所での呼び出され方を考えると、不用意なセッションの開始を避けるためのメソッドとしてあるべきかと思います。
現時点の実装のものを残すのであれば、少なくともメソッド名を変えたほうがよいかと思われます。下記のような名前でしょうか。
ただ、メソッドはRequestクラスではなくRequestListenerに持つ方が適切ではないかと思われます。