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

Already on GitHub? Sign in to your account

PdoSessionStorage - Symfony2.1 #1076

Merged
merged 5 commits into from Mar 7, 2012

Conversation

Projects
None yet
2 participants
Contributor

ManuelAC commented Feb 12, 2012

For more information please see 5b7ef11

Member

weaverryan commented Feb 14, 2012

Hi Manuel!

Yes, great change! But isn't the class name even simpler than in this change - simply PdoStorage?

Also, can we added a a little note about this change:

.. versionadded:: 2.1
    The PdoSessionStorage class was renamed to to `PdoStorage` in Symfony2.1 and it's namespace changed slightly.

Thanks!

Contributor

ManuelAC commented Feb 14, 2012

Good morning Ryan,

Clear, that's even easier and much cleaner than my solution. 👍

Although I think the message must be broaden to include all the changes:

  • Namespace - Storage vs SessionStorage
  • Argument swapping - %pdo.db_options% vs %session.storage.options%

My mind is at the moment completely blank on how to document this in a proper way. Any suggestions?
Thanks!

Member

weaverryan commented Feb 20, 2012

Hi Manuel!

Ok great. So here's what we need to do in order to merge this:

  • Add the above note. And yes, I think adding all of the details you mentioned is great.
  • Modify your changes - because there are still some class names that are incorrect. For example, it still says PdoSessionStorage when it should say PdoStorage

To make these changes your pull request, you can just add a few more commits to your 5b7ef11-update branch and push them to your fork. The new commits will automatically show up here.

Thanks!

Contributor

ManuelAC commented Feb 27, 2012

Hi Ryan!

Sorry for the late update, had a lot of work to do and almost forgot about this, my apologies again.

Just added the suggested changes you had, although I'm not sure if it's PdoStorage or PdoSessionStorage because my local dev version keeps saying that the class name is invalid. Are you sure that it's PdoStorage instead of PdoSessionStorage?

@weaverryan weaverryan added a commit that referenced this pull request Mar 7, 2012

@weaverryan weaverryan Merge pull request #1076 from IntoWebDevelopment/5b7ef11-update
PdoSessionStorage - Symfony2.1
c4eca61

@weaverryan weaverryan merged commit c4eca61 into symfony:master Mar 7, 2012

Member

weaverryan commented Mar 7, 2012

Hi Manuel!

My apologies! Yes, I gave you some bad advice - the class is called PdoSessionStorage - I think I was only considering the main PR and not other changes afterwards. Sorry about that - but you still led to this getting updated correctly. I put some of your changes back, along with some of my own in sha: 190c213

Thanks for the PR - hope to see more!

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