Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-31825: Fixed user with user:password unable to change password #117
EZP-31825: Fixed user with user:password unable to change password #117
Changes from 23 commits
5b3d545
79df0a1
67726e4
d5707d6
a0d7d5d
37886cb
0d367a0
8d1d838
1391111
072145c
a813e49
3a0c98b
b62e0f4
56b2c74
c030d83
8056fd6
dbebf22
463d857
9c19f6a
6e4fc65
4dc9e47
b1e425d
29df0f0
4e4b99b
f1a8591
18474be
02b7a78
5bca28b
63587c3
30b37cb
b625a0f
0ccda1e
d2dec1a
7cdb80e
f36240c
14a4a5b
b887eb0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Import the class, please. Other test cases w/o imports are the result of automatic Rector refactoring run and are waiting to be bulk-fixed.
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 I'm already working on this file I could do the update. Most of the comments regarding introduced tests result from me trying to keep to style consistent with other in the same file.
@alongosz 👍 / 👎 ?
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.
Depends on the diff. If it's too big -> separate PR. However new code should be aligned already, hence my comment
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.
A separate PR it is then.
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.
missing return type
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.
Interface that this class implements is missing return type for this API. It should be added even if other methods of this interface do not have them?
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.
Anyway, return type added.
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.
It depends. If we have control over the code, then definitely. In other cases it needs to be assessed (for instance if 3rd party type-hints in PHPDoc e.g.
@return bool
then it can be safely applied as well).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.
Serializing entire user struct[1] for logging was not the best choice in the method you've copied it from and is not the best choice here for user VO, due to password hashes. Maybe simply:
[1] Struct is not the best word here anyway, because it's a keyword in our Domain. What you have here is a Value Object. Though I see it's referred to everywhere as a "user struct"... :/
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 wasn't really sure if passing whole user structure into logger was important, so I've left it as-is (after copying). The same goes for
struct
key. I'll adjust accordingly, unless someone goes against it.