Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.8] [Bridge] [Doctrine] made session table columns configurable through constructor method. #14710

Closed
wants to merge 1 commit into from

Conversation

hhamon
Copy link
Contributor

@hhamon hhamon commented May 21, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ~
License MIT
Doc PR ~

@hhamon
Copy link
Contributor Author

hhamon commented May 21, 2015

This PR replaces #13136

@hhamon hhamon changed the title [2.8] [Bridge] [Doctrine] made session table columns configurable through constructor method. [WIP] [2.8] [Bridge] [Doctrine] made session table columns configurable through constructor method. May 21, 2015
@hhamon hhamon force-pushed the configurable-dbal-session-handler branch 2 times, most recently from 21a94cb to 88194e7 Compare May 21, 2015 03:07
@hhamon hhamon changed the title [WIP] [2.8] [Bridge] [Doctrine] made session table columns configurable through constructor method. [2.8] [Bridge] [Doctrine] made session table columns configurable through constructor method. May 21, 2015
@hhamon
Copy link
Contributor Author

hhamon commented May 21, 2015

ping @Tobion @stof is it ok now?

@hhamon hhamon force-pushed the configurable-dbal-session-handler branch from 88194e7 to 4126bee Compare May 21, 2015 03:23
@fabpot
Copy link
Member

fabpot commented May 21, 2015

Why would you want to make them configurable? What's the use case?

@Tobion
Copy link
Member

Tobion commented May 21, 2015

I don't think we should do this. Then we also have the problem that people can use reserved words which would need to be "escaped". See #11131

@Tobion
Copy link
Member

Tobion commented May 21, 2015

Also implementation wise, it would be better to accept a Doctrine\DBAL\Schema\Schema or Doctrine\DBAL\Schema\Schema\Table as argument for DbalSessionHandler. That would be cleaner and one only needs to configure the names in one place and not in two classes.

This way it would actually be possible to fix #11131 by using AbstractAsset::getQuotedName($connection->getDatabasePlatform) for the column name retrieval. But this should not be done in the constructor, but in open() to still be lazy-connecting as getDatabasePlatform might create the connection as we know.

@fabpot
Copy link
Member

fabpot commented May 22, 2015

@hhamon Closing as I agree with @Tobion.

@fabpot fabpot closed this May 22, 2015
@hhamon hhamon deleted the configurable-dbal-session-handler branch December 29, 2015 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants