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

EZP-29140: User bundle #278

Merged
merged 6 commits into from
Feb 28, 2019
Merged

Conversation

ViniTou
Copy link
Contributor

@ViniTou ViniTou commented Feb 19, 2019

Move and deprecate things based on
ezsystems/ezplatform-user#1

@ViniTou ViniTou force-pushed the EZP-29140-user-bundle branch from a9aa8da to 7a4105d Compare February 19, 2019 12:38
@ezsystems ezsystems deleted a comment from ezrobot Feb 19, 2019

class UserRegisterController extends Controller
/**
* @deprecated Deprecated in 1.5 and will be removed in 2.0. Please use \EzSystems\EzPlatformUserBundle\Controller\UserRegisterController instead.
Copy link
Member

Choose a reason for hiding this comment

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

I think the version is wrong here. It should be 2.5 and 3.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, it is 1.5 and 2.0 for adminUI, my bad.

- "@ezrepoforms.user_register.registration_group_loader.configurable"
calls:
- [setParam, ["language", "@=service('ezpublish.config.resolver').getParameter('languages', null, null)[0]"]]
alias: EzSystems\EzPlatformUser\Form\DataMapper\UserRegisterMapper
Copy link
Contributor

Choose a reason for hiding this comment

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

Add deprecated property here and in other services.

*/
class UserRegisterMapper
class UserRegisterMapper extends BaseUserRegisterMapper
Copy link
Contributor

Choose a reason for hiding this comment

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

By using inheritance this class will be prone to accidental BC breaks if something changes in parent class. I'd rather go for composition here to protect interface of this class.

@ViniTou ViniTou force-pushed the EZP-29140-user-bundle branch from 1a1e4e1 to 3392d6e Compare February 22, 2019 10:14
@ViniTou ViniTou requested a review from webhdx February 25, 2019 07:53
*
* @throws \Exception if the current user isn't allowed to register an account
* @return \EzSystems\EzPlatformUser\View\Register\FormView|\Symfony\Component\HttpFoundation\Response|null
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: @throws \Symfony\Component\HttpKernel\Exception\UnauthorizedHttpException

* Implementations will call load() with a repository callback as an argument.
* The repository can be accessed using getRepository().
*
* ** Use with care**.
Copy link
Contributor

Choose a reason for hiding this comment

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

because this is important, maybe it is worth to put it to the top

/**
* @var array
*/
private $params = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private $params = [];
private $params;

should it be null or array?

*/
private $params = [];

public function __construct(Repository $repository, $params = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: missing docblocks

abstract class ConfigurableSudoRepositoryLoader
{
/**
* @var Repository
Copy link
Member

Choose a reason for hiding this comment

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

CS: Please use FQCN

*/
private $params = [];

public function __construct(Repository $repository, $params = null)
Copy link
Member

Choose a reason for hiding this comment

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

CS: missing docblock

@lserwatka lserwatka merged commit 072a557 into ezsystems:master Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants