[DoctrineBridge][DBAL] Add PdoHandler-like $options #20288
Conversation
| { | ||
| $this->con = $con; | ||
| $this->table = $tableName; | ||
|
|
||
| $this->idCol = isset($options['db_id_col']) ? $options['db_id_col'] : $this->idCol; |
There was a problem hiding this comment.
What about removing the default value of the property and use it here instead, that would be clearer I think.
There was a problem hiding this comment.
That sounds like a good idea. I will update the PR with those changes and be sure to include unit tests within that same commit.
|
I moved the default values to the constructor for clarity and, added a test. I've also written documentation for the |
Small PR to incorporate the options that are available to `PdoSessionHandler` into the `DbalSessionHandler`. I do not like being forced to use pre-defined values, especially when they go against the code standards of my current project.
|
Honest question: why would someone use DbalSessionHandler vs PdoSessionHandler?
|
|
That is very good question! What I know, which isn't a lot, is that this is used purely for the Doctrine ORM. This means that you do not need to mess with the database to add a session table and everything will be handled through Doctrine itself.
Could you explain this statement? As far as I know I'm using the same connection. Either way, even if this is worse, the discussion to keep it or not (which is what you seem to be implying) doesn't belong here nor does it affect this PR. Making something more configurable is a good thing, right? |
|
Of side note: the cache component has a PdoAdapter that looks a lot like the PdoSessionHandler; but it also works with DBAL connections (there is no DbalAdapter). Could have been done here also I guess. |
|
@Hipio the first point is a justification for the Schema integration, but this does not justify the handler. The benefit of the DBAL handler is support for non-PDO drivers of DBAL. But this is the only benefit IMO, and the cache adapter showed that the support could be added in PDOHandler directly (as the DBAL interface matches closely the PDO one)
if you use the same connection, you will face issues, as the session handlers relies on using transactions. |
|
All I want to do is pass options to the DbalSessionHandler because I do not want to be use the pre-defined values. If this PR does not contain the right solution to do this, I would like to know what a better solution would be. |
|
@Hipio This PR does the job :) Sorry for having highjacked your PR with a side discussion that it indeed totally orthogonal to the changes you propose here. We are in code freeze right now, so this PR will be merged after the release of 3.2. |
|
This was already proposed before, see arguments in #13136 |
|
The main arguments against this change are there: #14710#issuecomment-104189679 Closing then. |
|
@Tobion states:
But I'm not entirely sure I understand what is being said in that thread. As @Hipio said:
I would also like to know, if this is not the way to go, what is the way to implement a fix that will be accepted for configuring the DbalSessionHandler? |
|
@Potherca we started a discussion about the fact that DbalSessionHandler itself is not the way to go. Switch to the PdoSessionHandler, and you will win the configurability. |
|
The application I am working with uses Doctrine for everything, this includes sessions. I looks like switching to the PdoSessionHandler will gain me configurability at the cost of loosing the functionality of having session database handling being done by Doctrine. Or am I missing something? |
|
Also, does my comment belong here or should I move it over to #20501? |
Small PR to incorporate the options that are available to
PdoSessionHandlerinto theDbalSessionHandler. I do not like being forced to use pre-defined values, especially when they go against the code standards of my current project.