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

Remove FOSUserBundle #1449

Merged

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented Dec 9, 2021

Subject

I am targeting this branch, because this will break bc.

Closes #1256 #1112

This PR tries to remove FOSUserBundle with the minimal code needed:

  • Remove FOSUserBundle from dependencies
  • Add missing files from FOSUser that are needed to make test green
  • Add missing translations here
  • Pass all the Github Actions defined

This PR will not (Those will go in separate PRs once this one is accepted, and only if really needed):

  • Add support for more Symfony versions
  • Reduce the UserInterface
  • Remove / Deprecate the groups feature
  • Add commands or another features from FOSUserBundle that are not mandatory to pass tests
  • Use symfonycasts/reset-password-bundle to reset password

This PR needs: #1444

I tried to be consistent with what is done on other bundles with the managers and entities and models. (I also discovered some things that can be improved on media-bundle and probably on classification-bundle).

Keep in mind that this code can change in nexts PRs when we need to add more things like commands or other things. This is just a first version to be able to remove FOSUserBundle and enable next Symfony versions. It also does not make sense to port this to stable branch, because it will be a huge effort for not that much benefit.

Changelog

### Removed
- Removed FOSUserBundle dependency.

@jordisala1991 jordisala1991 changed the base branch from 4.x to 5.x December 9, 2021 11:16
@jordisala1991 jordisala1991 force-pushed the feature/remove-fos-user branch 2 times, most recently from 904d68c to 8b5d2c3 Compare December 9, 2021 14:21
@jordisala1991 jordisala1991 marked this pull request as ready for review December 9, 2021 15:43
@jordisala1991 jordisala1991 force-pushed the feature/remove-fos-user branch 2 times, most recently from 571416f to af88dcf Compare December 9, 2021 16:17
@jordisala1991
Copy link
Member Author

jordisala1991 commented Dec 9, 2021

Things missing:

  • Canonicalization of fields
  • Make sure nothing of FOS is left
  • Check if LoginManager is really needed, only used on ResetAction
  • Add LastLogin listener
  • Check validation
  • Add Remember me compiler pass (if LoginManager is needed)
  • Check Symfony requirements
  • Add missing configuration options present in FOSUser (need to remove the hardcoded values)
  • Check if some unit test can be copied from FOSUser
  • Move english translations

@jordisala1991 jordisala1991 force-pushed the feature/remove-fos-user branch from 58ef20d to f1a1b3d Compare December 9, 2021 19:10
@jordisala1991 jordisala1991 force-pushed the feature/remove-fos-user branch from f1a1b3d to 4ea11ae Compare December 9, 2021 19:34
@jordisala1991 jordisala1991 force-pushed the feature/remove-fos-user branch 2 times, most recently from fb97c32 to be283c3 Compare December 12, 2021 14:43
@jordisala1991 jordisala1991 changed the title [WIP] Remove FOSUserBundle Remove FOSUserBundle Dec 16, 2021
@jordisala1991
Copy link
Member Author

jordisala1991 commented Dec 16, 2021

I would like to get some reviews, @sonata-project/contributors . The PR is mostly done, the code should be okay. (Remember that we are not trying to have one PR that lets this bundle perfect, but we are only trying to remove FOSUser)

I copied the needed translations only for english to validate we are not missing any.

@Hanmac or @eerison , or really any other user of this bundle: If you can try this PR in your projects it will be helpful too (Note that even if we removed FOSUser, this bundle is still not compatible with SF 6, it will be on a future PR to not grow this one bigger and bigger)

@jordisala1991 jordisala1991 requested review from VincentLanglet and a team December 16, 2021 13:05
VincentLanglet
VincentLanglet previously approved these changes Dec 16, 2021
@VincentLanglet VincentLanglet requested a review from a team December 16, 2021 13:08
Copy link
Member

@wbloszyk wbloszyk left a comment

Choose a reason for hiding this comment

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

@sonata-project/contributors
WDYT about reduce UserBundle only to integrate User with Admin bundle?
IMO we should not provide a fully user management but only expands Symfony user (or Sonata User) by Groups.

@jordisala1991
Copy link
Member Author

jordisala1991 commented Dec 17, 2021

@sonata-project/contributors
WDYT about reduce UserBundle only to integrate User with Admin bundle?
IMO we should not provide a fully user management but only expands Symfony user (or Sonata User) by Groups.

I am not sure to follow you on what needs to be changed then. IMHO I have reduced to only a minimal integration with User and Admin.

And please, keep in mind this Pr is big enough and it is not intended to hold all the code changes needed to release a new version. Check my next PR to see how easy it is to add sf 5 and 6 once this one is merged: #1454

@wbloszyk
Copy link
Member

@sonata-project/contributors
WDYT about reduce UserBundle only to integrate User with Admin bundle?
IMO we should not provide a fully user management but only expands Symfony user (or Sonata User) by Groups.

I am not sure to follow you on what needs to be changed then. IMHO I have reduced to only a minimal integration with User and Admin.

And please, keep in mind this Pr is big enough and it is not intended to hold all the code changes needed to release a new version. Check my next PR to see how easy it is to add sf 5 and 6 once this one is merged: #1454

So I start working on POC in 4.x.

@jordisala1991
Copy link
Member Author

@sonata-project/contributors
WDYT about reduce UserBundle only to integrate User with Admin bundle?
IMO we should not provide a fully user management but only expands Symfony user (or Sonata User) by Groups.

I am not sure to follow you on what needs to be changed then. IMHO I have reduced to only a minimal integration with User and Admin.
And please, keep in mind this Pr is big enough and it is not intended to hold all the code changes needed to release a new version. Check my next PR to see how easy it is to add sf 5 and 6 once this one is merged: #1454

So I start working on POC in 4.x.

To me the work is already done, if you have some specific things to change, please add your comments, but do not block the PR.

@wbloszyk
Copy link
Member

wbloszyk commented Dec 17, 2021

@sonata-project/contributors
WDYT about reduce UserBundle only to integrate User with Admin bundle?
IMO we should not provide a fully user management but only expands Symfony user (or Sonata User) by Groups.

I am not sure to follow you on what needs to be changed then. IMHO I have reduced to only a minimal integration with User and Admin.
And please, keep in mind this Pr is big enough and it is not intended to hold all the code changes needed to release a new version. Check my next PR to see how easy it is to add sf 5 and 6 once this one is merged: #1454

So I start working on POC in 4.x.

To me the work is already done, if you have some specific things to change, please add your comments, but do not block the PR.

@jordisala1991
Can you look at: #1455
It is only example.

@jordisala1991
Copy link
Member Author

@sonata-project/contributors

WDYT about reduce UserBundle only to integrate User with Admin bundle?

IMO we should not provide a fully user management but only expands Symfony user (or Sonata User) by Groups.

I am not sure to follow you on what needs to be changed then. IMHO I have reduced to only a minimal integration with User and Admin.

And please, keep in mind this Pr is big enough and it is not intended to hold all the code changes needed to release a new version. Check my next PR to see how easy it is to add sf 5 and 6 once this one is merged: #1454

So I start working on POC in 4.x.

To me the work is already done, if you have some specific things to change, please add your comments, but do not block the PR.

@jordisala1991

Can you look at: #1455

It is only example.

I have checked it and I think it is not related to this PR. You are trying to introduce a new integration not needed to remove FOSUser. An integration that has to be well thought because of the issues I mentioned.

IMO we should:

  1. Finish this PR to unlock more Pr: like sf 5 and 6 support
  2. Add sf 5 and 6 support ( Pr already opened)
  3. Increase phpstan, psalm
  4. Increase php version
  5. ...

And probably one point of that is about symfony casts reset password bundle.

@wbloszyk wbloszyk dismissed their stale review December 17, 2021 11:17

I dont wanna block this PR

Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

There is maybe some improvements to do @wbloszyk but I think @jordisala1991 made a really good job here to help us to move forward with this project.

After this we will be able to add Symfony 5 support and improve the code quality. And we can still add things like your POC. That's not incompatible.

@VincentLanglet VincentLanglet requested a review from a team December 17, 2021 13:08
@jordisala1991 jordisala1991 merged commit 3969ef8 into sonata-project:5.x Dec 17, 2021
@jordisala1991 jordisala1991 deleted the feature/remove-fos-user branch December 17, 2021 15:13
@jordisala1991
Copy link
Member Author

Let's have this one and improve on next PRs.

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

Successfully merging this pull request may close these issues.

4 participants