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

Support class inheritance in UserProvider::supportsClass by default #532

Merged
merged 1 commit into from
Oct 3, 2020

Conversation

MisatoTremor
Copy link
Contributor

I propose to add the check if given class name is inherited from the User class (like the EntityUserProvider implementation does) to the UserProvider template.

Also I would follow up with a documentation PR reflecting that change in https://github.com/symfony/symfony-docs/blob/master/security/user_provider.rst

Or would it better to just add a info about this additional check to the documentation?

Check if given class name is inherited (i.e. Doctrine Proxies) from the User class like the default implementation does.
@wouterj
Copy link
Member

wouterj commented Jan 24, 2020

There is nothing directly wrong with doing AppUser::class === $class. There is something wrong if you have multiple user classes extending from AppUser or if you do UserInterface::class === $class.

So I'm leaning towards keeping the current AppUser::class === $class template: It's what most people need, doesn't involve a function call and making this call more strict is better than making it too loose.

@MisatoTremor
Copy link
Contributor Author

And it's wrong if e.g. using an ORM to load the user or other cases where you can end up getting a proxy object (which seems to be the main issue lately).

As for the strictness, that's indeed a valid point ...

My alternate proposition was to add a note to the Custom User Provider documentation (and maybe add a link to that in the nextSteps message). How about that?

@wouterj
Copy link
Member

wouterj commented Jan 24, 2020

And it's wrong if e.g. using an ORM to load the user or other cases where you can end up getting a proxy object (which seems to be the main issue lately).

Oh, I missed this. Then definitely 👍 (99.999% of the applications uses some sort of Doctrine library to save the user I think)

@javiereguiluz
Copy link
Member

I also suffered this problem. Every page load resulted in this error: There is no user provider for user "Proxies\__CG__\App\Entity\User. After a page reload, the error disappears ... but that means that I must load each page twice to use my app.

@linaori kindly pointed me to this PR, so I applied the proposed fix and everything works now as it should. Please, consider merging this as soon as possible. Thanks!

@joshlopes
Copy link

Same issue as @javiereguiluz mentioned, it was pointed to me by someone from support sf channel. It appeared with the new sf minor update -- tests started failing on my app.

This fixed the issue, thanks.

@weaverryan
Copy link
Member

Thanks @MisatoTremor! Sorry for the delay - I don't know how I missed this PR!

@weaverryan weaverryan merged commit faaedb4 into symfony:master Oct 3, 2020
wouterj pushed a commit to CharlyPoppins/symfony-docs that referenced this pull request Oct 3, 2020
wouterj added a commit to symfony/symfony-docs that referenced this pull request Oct 3, 2020
This PR was submitted for the master branch but it was merged into the 4.4 branch instead.

Discussion
----------

minor [#14292] UserProvider::supportsClass

symfony/maker-bundle#532

Commits
-------

2b02a0a minor [#14292] UserProvider::supportsClass
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.

5 participants