Skip to content

Updated "Load security users from database" #1874

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Updated "Load security users from database" #1874

wants to merge 4 commits into from

Conversation

alex88
Copy link
Contributor

@alex88 alex88 commented Oct 31, 2012

I've updated the "How to load Security Users from the Database" cookbook to include fix this error:

PHP Fatal error:  Call to a member function getRole() on a non-object in /vendor/symfony/symfony/src/Symfony/Component/Security/Core/Role/RoleHierarchy.php on line 47

which is happening in PHP > 5.4.
The fix is taken by this issue: symfony/symfony#3691 (comment)

@alex88
Updated "Load security users from database"
f9ce55f 
I've updated the "How to load Security Users from the Database" cookbook to include fix this error:

PHP Fatal error:  Call to a member function getRole() on a non-object in /vendor/symfony/symfony/src/Symfony/Component/Security/Core/Role/RoleHierarchy.php on line 47

which is happening in PHP > 5.4.
The fix is taken by this issue: symfony/symfony#3691 (comment)
wouterj
wouterj reviewed Oct 31, 2012
View reviewed changes
cookbook/security/entity_provider.rst
*/
public function serialize()
{
return \serialize(array(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need a \ prefix if you use a function?

@alex88
Copy link
Contributor Author

alex88 commented Oct 31, 2012

Ok, fixing them now

@weaverryan
Copy link
Member

Hi Alessandro!

I agree that this is a good note to add :). But, I believe if you're using the entity provider (i.e. EntityUserProvider), then you only need to serialize the id column, which is then used to re-query for the user in EntityUserProvider::refreshUser. What do you think?

I also think that our implementation of UserRepository::refreshUser should be modified to use the primary key instead of the username (basically, doing what EntityUserProvider does).

Let me know if you agree! And if so, let's at least make the changes to only serialize the id - I can make the second change if you like :).

Thanks for bringing this to attention!

@alex88
Copy link
Contributor Author

alex88 commented Nov 17, 2012

Actually I don't remember why I've serialized all the fields of the user, maybe to pass it from a request to another saving it in the session...

But generally I think it's ok to just serialize the id for most of the cases...

I'll do the changes requested, don't worry. I'm on the ipad so I'll try with the github editor, if it doesn't work I'll turn on the pc later.. Just have some patience ;)

@alex88
Using id for serializing, refreshUser with id
e6791dc 
Serialize with only the id field, refreshed user is called by id
@alex88
Copy link
Contributor Author

alex88 commented Nov 17, 2012

The refreshUser is ok this way? Else feel free to comment or edit ;)

@weaverryan
Copy link
Member

Hi Alessandro!

What is the purpose of adding the serialize to the Group entity? If the User is serialized, and we have a Serializable interface that only serializes the id of User, then what is the point of the Group serialization stuff? There might be a good reason, I just don't see it, so I'm asking before the merge!

Thanks!

@alex88
Copy link
Contributor Author

alex88 commented Nov 20, 2012

Hi Ryan,
What I've done is to implement the fix that I've referenced in the description, since the default docs with PHP 5.4 doesn't work fine.
So I'm note sure that the group serialization is really needed, but I'll let you know tomorrow since I'll test the whole thing without the group serialization.

@alex88
Copy link
Contributor Author

alex88 commented Nov 21, 2012

@weaverryan ok removed the serialisation from the Group entity, seems to still works fine.

weaverryan added a commit that referenced this pull request Nov 22, 2012
@weaverryan
[#1874] Tweaking the entity provider article, including a new explana…
5547d66 
…tion of why serialization might be important
@weaverryan
Copy link
Member

Hi Alessandro!

Ah, great - thanks so much for testing this without the Group serialization and then updating the article. It's now a great addition, and contains only the needed details! I've patched your commits into the 2.0 branch at the following commits (I'll merge 2.0 into 2.1 and master shortly):

I also made a few very small tweaks.

Thanks!

@weaverryan weaverryan closed this Nov 22, 2012
@alex88
Copy link
Contributor Author

alex88 commented Nov 22, 2012

Oh true, nice tweaks! Thanks!

@alex88 alex88 mentioned this pull request Jan 2, 2013
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alex88 @weaverryan @wouterj