Skip to content

[WIP] Added DBAL session storaged based on PDO Session storage page #3914

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

Closed
wants to merge 7 commits into from
Closed

[WIP] Added DBAL session storaged based on PDO Session storage page #3914

wants to merge 7 commits into from

Conversation

xocasdashdash
Copy link

Q A
Doc fix? no
New docs? yes
Applies to 2.1
Fixed tickets #881

I added a quick reference to https://github.com/symfony/symfony/blob/2.1/src/Symfony/Bridge/Doctrine/HttpFoundation/DbalSessionHandler.php basing it almost completely on the PDO Session handler

I'm merging it into this branch, even though it's not mantained anymore, because this is where this option became available to use for session storage

@Tobion
Copy link
Contributor

Tobion commented Jun 6, 2014

I don't think having a cookbook for each makes sense because so much duplication. Instead both should be merged and the title more general.

@weaverryan
Copy link
Member

Hi Joaquín!

Thanks very much for adding this! I agree with @Tobion that we should modify the existing cookbook entry and add details on how you would use the DBAL to it. We can certainly change its title to be more generic.

What do you think?

Thanks!

@xocasdashdash
Copy link
Author

I completely agree. I'll mix my changes into the old file.
About the title, does "How to store Session in the database" sound ok?

@xabbuh
Copy link
Member

xabbuh commented Jun 10, 2014

About the title, does "How to store Session in the database" sound ok?

Sounds good to me. Though, according to our standards it should be written "How to Store Session in the Database".

@wouterj
Copy link
Member

wouterj commented Sep 16, 2014

ping @xocasdashdash Can you please update this PR? (if you don't have time, feel free to say so, then somebody else will take it over from you)

@xocasdashdash
Copy link
Author

I will get on it this week and deliver an update by Saturday.

2014-09-16 15:03 GMT+02:00 Wouter J notifications@github.com:

ping @xocasdashdash https://github.com/xocasdashdash Can you please
update this PR? (if you don't have time, feel free to say so, then somebody
else will take it over from you)


Reply to this email directly or view it on GitHub
#3914 (comment).


Joaquín Fernández Campo
E-Mail: jfcampo@gmail.com

@xocasdashdash
Copy link
Author

ping @wouterj . Any news on these?
Github says there's a merge conflict but I can't find it

@@ -1,7 +1,7 @@
.. index::
single: Session; Database Storage

How to use PdoSessionHandler to store Sessions in the Database
How to use Store Session in the Database
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would need to remove the "use".

@weaverryan
Copy link
Member

@xocasdashdash Sorry for the delay! I've just added some feedback :).

Cheer!

@xocasdashdash
Copy link
Author

No worries!
I'll try to fix everything tomorrow. Another question, any idea how can I see the way the docs are gonna look online? I'm flying blind right now

@wouterj
Copy link
Member

wouterj commented Oct 18, 2014

@xocasdashdash you can't (we're working on that). You can however install python and sphinx locally and render it using a default sphinx theme. See http://symfony.com/doc/current/contributing/documentation/overview.html for more information.

And if you rebase (git rebase origin/master, where origin is this original repository), the PR gets a travis build which tests for errors.

@wouterj
Copy link
Member

wouterj commented Oct 31, 2014

Ping?

@xocasdashdash
Copy link
Author

I'm gonna work on some of the changes proposed by @weaverryan, I agree with most of them, and squash the different commits into one

@Tobion
Copy link
Contributor

Tobion commented Nov 2, 2014

FYI, in master (upcoming 2.6) the enhancements for PdoSessionHandler (symfony/symfony#10931) are not yet implemented for DoctrineSessionHandler. So currently they are quite different in terms of feature set.

@xocasdashdash
Copy link
Author

@Tobion mmm I see there's quite a bit of discussion on that thread. I'm thinking of how to add this info here, but as @weaverryan said, choices suck unless we can help them decide.
I'll keep on working on this merge this week and will update asap

@wouterj
Copy link
Member

wouterj commented Nov 4, 2014

yes, I agree

@xocasdashdash
Copy link
Author

Ok, I'll rework @weaverryan last proposal changes into the docs

@wouterj
Copy link
Member

wouterj commented Dec 28, 2014

@xocasdashdash can you please update this PR?

The DbalSessionHandler has not been updated for 2.6. So I believe the recommendation is now to use the PdoSessionHandler.

@xocasdashdash
Copy link
Author

I won't be able to until the 15th or so of this month. Is that workable?

@xabbuh
Copy link
Member

xabbuh commented Dec 30, 2014

@xocasdashdash Of course, we know that everyone does the work on the docs in their spare time. We are really looking forward to see you back soon. However, feel free to ping us if you think you won't have the time and we'll find someone who finishes your great work.

@xabbuh
Copy link
Member

xabbuh commented Mar 8, 2015

@xocasdashdash Do you think you can finish this? :)

@xocasdashdash
Copy link
Author

@xabbuh Ok! If i don't finish it this week don't hesitate on rejecting/closing it and assigning it to somebody else!

@xocasdashdash
Copy link
Author

@xabbuh Ok, I think i'm finished, how can I check that everything's correct format wise? How should I proceed?

To use The DBAL session storage, you need to register a new service and configure
Symfony's session handling to use it:


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please remove on of these 2 empty lines?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@wouterj
Copy link
Member

wouterj commented Mar 9, 2015

@xocasdashdash I've just added some formatting comments. If you fix those, you should be ready to go.

There is also a merge conflict, can you please rebase onto 2.3 (As that's the branch where this PR is going to be merged into).

Assuming origin points to this repository and xocasdashdash to your fork:

$ git fetch origin
$ git rebase --onto origin/2.3 origin/master
$ git push -f including_dbalsession_doc xocasdashdash

If you've solved the conflict, this PR can also be build by Travis, which checks for syntax errors.

Btw, why did you create a dbal_session_storage article with the same content as the new database_session_storage article?

@xocasdashdash
Copy link
Author

@wouterj I've tried to fix the merge conflicts, but I can't seem to get it right 😕

@@ -1,19 +1,153 @@
.. index::
single: Session; Database Storage

How to Use PdoSessionHandler to Store Sessions in the Database
==============================================================
How to Store Session in the Database
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sessions

@xabbuh
Copy link
Member

xabbuh commented Mar 21, 2015

@xocasdashdash To be build on Travis, your pull request has to be rebased onto the branch you want to merge in (master in your case). Alternatively, you can open a new pull request with your (already rebased on the 2.3 branch) changes for the 2.3 branch.

@wouterj
Copy link
Member

wouterj commented May 15, 2015

Hi @xocasdashdash!

Do you have time to update your PR? We have a doc sprint day on May 23rd and we would like to finish as much PRs as possible before/during that PR.

Don't worry if you have no time soon, we know it's all freetime work. @javiereguiluz already started a new PR without knowing this PR existed: #5019 you can also comment on that PR with some details that you think are missing from his PR and close this PR.

If you need any help, you can almost always find one of us on the #symfony-docs IRC freenode channel.

Thanks!

@xocasdashdash
Copy link
Author

@wouterj I've checked @javiereguiluz PR and it looks fine by me.

I'm gonna close down this PR then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants