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

feat: add UserModel::getReturnType() and RegisterController::getUserEntity() uses it #1172

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

christianberkman
Copy link
Contributor

@christianberkman christianberkman commented Aug 18, 2024

Description
See #1170
Refactor RegisterController::getUserEntity to use UserModel::returnType instead of the hard coded new User(). This allows for easy use of a custom User Entity without needing to extend the RegisterController and add the routes.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@christianberkman christianberkman changed the title User entity refactor RegisterController::getUserEntity() to use UserModel::getReturnType() Aug 18, 2024
@christianberkman christianberkman changed the title refactor RegisterController::getUserEntity() to use UserModel::getReturnType() refactor: RegisterController::getUserEntity() to use UserModel::getReturnType() Aug 18, 2024
@kenjis kenjis added the enhancement New feature or request label Aug 19, 2024
@@ -395,4 +395,9 @@ private function checkReturnType(): void
throw new LogicException('Return type must be a subclass of ' . User::class);
}
}

public function getUserEntity(): User
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function getUserEntity(): User
/**
* Returns User Entity Type
*
* @return class-string<User>
*/
public function getReturnType(): string

Copy link
Member

Choose a reason for hiding this comment

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

Is it better to add createNewUser(array $data): User or something instead of getReturnType(): string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would omit the UserEntity entirely so we would still not be able to use UserEntity during the RegistrationAction?

Copy link
Member

Choose a reason for hiding this comment

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

If we add the following method to UserModel:

    /**
     * Returns a new User Entity.
     *
     * @param array<string, mixed> $data The data passed to the constructor
     */
    public function createNewUser(array $data = []): User
    {
        return new $this->returnType($data);
    }

RegisterController would be like this:

    protected function getUserEntity(): User
    {
        $userProvider = $this->getUserProvider();

        return $userProvider->createNewUser();
    }

Copy link
Member

Choose a reason for hiding this comment

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

@christianberkman
I think adding a method to create a new user is better than a method that just returns the user type string.
Would you like to make such an update?

Copy link
Contributor Author

@christianberkman christianberkman Aug 27, 2024

Choose a reason for hiding this comment

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

Sorry typo. I meant

So do you propose the following? (My last code snippet)

I also read your snippet with createNewUser wrong and actually we are on the same line indeed.

Is it now better to open a fresh pull request since this differs from the original suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would be better to open a fresh PR.

And I propose to add the first parameter:

    /**
     * Returns a new User Entity.
     *
     * @param array<string, mixed> $data (Optional) user data
     */
    public function createNewUser(array $data = []): User
    {
        return new $this->returnType($data);
    }

Then, a dev can create a complete new User entity.
If a dev want to use Complete Constructor to the custom User entity, s/he needs complete data to construct a new entity.
Also, the CI4 Entity permits an empty instance creation, so we can just call it without arguments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally don't like using mixed, and I prefer to explicitly define the possible types as much as we can. For any other unexpected values, we can wait for bug reports from users.

Copy link
Member

Choose a reason for hiding this comment

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

If we don't use mixed, something like this:
array<string, array|bool|int|float|null|object|string>

But it contains array, and it is not acceptable. Therefore,
array<string, array<array-key, array|bool|int|float|null|object|string>|bool|int|float|null|object|string>

But still contains array... Give up and use mixed.
array<string, array<array-key, mixed>|bool|int|float|null|object|string>

Which is better?

  • @param array<string, mixed> $data (Optional) user data
  • @param array<string, array<array-key, mixed>|bool|int|float|null|object|string> $data (Optional) user data

or something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My choice is the second option because it strikes a balance between overly vague type definitions and overwhelming complexity. While it's not ideal, it offers a reasonable level of flexibility and clarity.

@param array<string, array<array-key, mixed>|bool|int|float|null|object|string> $data (Optional) user data

@kenjis kenjis changed the title refactor: RegisterController::getUserEntity() to use UserModel::getReturnType() feat: add UserModel::getReturnType() and RegisterController::getUserEntity() uses it Aug 19, 2024
@kenjis
Copy link
Member

kenjis commented Sep 8, 2024

I sent #1196
Check, please.

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

Successfully merging this pull request may close these issues.

3 participants