-
Notifications
You must be signed in to change notification settings - Fork 348
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
Establish entity interface and database service for login tokens. #3667
Conversation
* | ||
* @return LoginTokenRow | ||
*/ | ||
public function saveToken( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this method has never been included in released code, I decided it was safe to delete it now that the logic has been refactored to the LoginTokenService, rather than going to the trouble of retaining a deprecated version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks otherwise good to me, however the signature of DbServiceInterface::saveToken()
seems a bit odd. I would expect a saveToken()
method to accept a LoginTokenEntityInterface
, instead of creating one.
I think I would rather have a simple saveToken(LoginTokenEntityInterface $token)
and have the caller create and populate the token. Or offer both possibilities with an additional createAndSaveToken()
with the current saveToken()
parameters.
@aleksip, I don't think there's a need for a I also notice that there's a fairly similar |
@demiankatz Oops, I missed This of course changes things slightly, although |
@aleksip, the difference between In any case, I like your idea about renaming the method to say "Persist" instead of Save." I'll take care of that in a moment. |
@demiankatz Ah, yes of course. But is it really possible and/or good that e.g. a call like |
I think it depends on the level of granularity of the services we create. If we want to strictly say that each service class can only deal with one type of entity, then it would make sense to be more restrictive. However, there are some edge cases where flexibility may be justified -- especially situations where linking tables are involved. For example, if we want to add a new resource to a favorite list, do we want to require access to the ResourceService and the UserResourceService to accomplish this, or might it be better to allow the ResourceService or the UserService to do all of the work? I'm inclined to think requiring separate service access for linking tables may be overcomplicated, and that's the main reason I've left persistEntity open-ended for now. There's certainly an intermediate approach where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@demiankatz Makes sense, especially the not painting ourselves into corners part. Maybe one option would be to have separate higher level services for the cases where linked tables are needed? Anyway, this PR LGTM!
Yes, it's definitely possible that we'll need some separate higher-level services to reorganize things after the first round of refactoring is done. I'm keeping an open mind about that -- just a question of how to namespace/organize things if we do find that we have that need. |
No description provided.