-
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
Expand use of UserEntityInterface in console commands #3638
Expand use of UserEntityInterface in console commands #3638
Conversation
a793b1b
to
37d3ad7
Compare
? $oldcipher->decrypt($row->getCatPassEnc()) | ||
: $row->getRawCatPassword(); | ||
$row->setRawCatPassword(null); | ||
$row->setCatPassEnc($pass === null ? null : $newcipher->encrypt($pass)); |
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.
We can't change typehints to the entity interfaces yet because of the use of ->save() below. Addressing that will require more intrusive changes to this class, which I'll take care of as a future action.
public function setCreated(DateTime $dateTime): UserEntityInterface; | ||
|
||
/** | ||
* Created getter |
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.
Nit: period or no period? The other one has, this doesn't..
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.
Do you have a preference? This is inconsistent throughout the codebase (see also the last login getter and setter above). My gut feeling is "period at the end of a complete sentence, no punctuation otherwise" but I'm open to other approaches. Whatever we decide on, it's going to be difficult to apply the rule globally, but we can at least try to clean up the entity-related code.
In any case, I'm not going to let this hold up merging this PR, since the issue is bigger than the changes here anyway and will require follow-up work.
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.
I've no strong preference, but I'd try to be consistent at least at class level. Your gut feeling sounds about right.
This PR expands the use of UserEntityInterface in console commands to enable more platform-independent mocking in tests and for greater forward-compatibility. This work is extracted from #3629 and is not exhaustive; it is just enough change to make tests pass in that PR. Nonetheless, it seemed like a worthwhile incremental step forward that made sense to commit independently.