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

Drop FOSUserBundle with minimum functionality #1256

Closed
wants to merge 10 commits into from

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented Oct 15, 2020

Subject

This PR move minimul functionality from FOSUserBundle which will be allow using SonataUserBundle without FOSUserBundle dependencies/inheritance.

I am targeting this branch, because this change are BC-BREAK.

Closes #1255

Changelog

### Added
- Added some `Class::newMethod()` to do great stuff.

### Changed

### Deprecated

### Removed

### Fixed

### Security

To do

  • Update the tests;
  • Update the documentation;
  • Add an upgrade note.

@wbloszyk wbloszyk requested a review from a team October 15, 2020 17:11
@wbloszyk wbloszyk changed the title Minimum drop fos user Drop FOSUserBundle with minimum functionality Oct 15, 2020
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@@ -23,7 +23,6 @@
"require": {
"php": "^7.2",
"doctrine/common": "^2.0",
"friendsofsymfony/user-bundle": "^2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

@wbloszyk I removed FosUserBundle from other project and I needed to add

  • symfony/security-bundle ("maybe" you don't need this because we are adding symfony/security-core)
  • doctrine/annotations
  • symfony/validator

@wbloszyk wbloszyk force-pushed the minimum_drop_fos_user branch from 11aee55 to 90076b5 Compare February 8, 2021 14:30
@wbloszyk wbloszyk force-pushed the minimum_drop_fos_user branch from d95e577 to 8e996a5 Compare February 8, 2021 14:38
@wbloszyk
Copy link
Member Author

wbloszyk commented Feb 8, 2021

@sonata-project/contributors Can you check it? IMO it is good star point to work on SonataUserBundle v4.

Comment on lines -38 to +26
class: # Entity Classes
user: Application\Sonata\UserBundle\Entity\User
group: Application\Sonata\UserBundle\Entity\Group
class: # Entity Classes
user: 'App\Entity\SonataUserUser'
group: 'App\Entity\SonataUserGroup'
Copy link
Member

Choose a reason for hiding this comment

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

Previously the entity was in the UserBundle, now it have to be created by the user ?
Cant/Shouldnt we provide a User class ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previus we are using EasyExtendsBundle to do it in Application\Sonata\UserBundle\Entity\User.
IMO in UserBundle v4 we should provide recipies with default entities. Using Sonata\UserBundle\Entity\BaseUser will be also allowed.

Copy link
Member

Choose a reason for hiding this comment

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

So we shouldn't use 'App\Entity\SonataUserUser' in the config
But 'Sonata\UserBundle\Entity\User'

Copy link
Member Author

Choose a reason for hiding this comment

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

I recheck it. It is not possible to use Sonata\UserBundle\Entity\BaseUser. I think it is not possible to provide User class in easy way. We should not provide User identifier by own and can not use two User classes for one entity. So we must keep App\Entity\SonataUserUser

Copy link
Member

Choose a reason for hiding this comment

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

Why ?

You still provide the classes BaseUser and a BaseGroup. What's wrong with them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so we should add a note about the interface that the customUser/Group need to implements and be sure it's clear we provide a Base class to help developers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not work on upgrade note jet. I waiting for appove current change. Then I will update note based on approved changes.

$container->setAlias('sonata.user.util.email_canonicalizer', $config['service']['email_canonicalizer']);
$container->setAlias('sonata.user.util.username_canonicalizer', $config['service']['username_canonicalizer']);
$container->setAlias('sonata.user.util.token_generator', $config['service']['token_generator']);
//$container->setAlias('sonata.user.user_manager', new Alias($config['service']['user_manager'], true));
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

This utils was moved from FOSUserBundle (to keep the same functionality like with FOSUSer). In next PRs we can remove it.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, why is there a line commented ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Becouse it this alias is create in line 305. For now I do not know what to do with it.

@wbloszyk wbloszyk requested review from VincentLanglet and a team February 8, 2021 15:33
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@Webonaute
Copy link

any advancement on this?

@VincentLanglet
Copy link
Member

any advancement on this?

Feel free to overtake the pr

@wbloszyk wbloszyk force-pushed the minimum_drop_fos_user branch from 7fa7c50 to 351a5c0 Compare May 21, 2021 14:06
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?


パスワードをリセットするには、次のリセット URL へアクセスしてください %confirmationUrl%


Choose a reason for hiding this comment

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

  • yamllint: too many blank lines (2 > 1) (empty-lines)

@VincentLanglet VincentLanglet marked this pull request as draft June 13, 2021 16:07
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@VincentLanglet
Copy link
Member

Closed in favor of #1449

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.

6 participants