-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
add method to user-backend to check if allowed to update email address #19053
Conversation
makes sense 👍 |
* @return bool | ||
* @since 8.2.0 | ||
*/ | ||
public function canChangeEMailAddress(); |
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.
Will this break existing backends that might not implement it ? (ex: in third party apps that might implement this interface)
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.
@blizzz Was this the case when you added public function canChangeAvatar();
?
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.
@georgehrke I did not add it into an interface. This was added by @icewind1991. Anyway, every backend will break, which implements IUser and does not adopt in time to this change.
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.
So, yes, it'll break user backends, which for whatever reason implement iUser, but it's just a matter of implementing a single method that returns a boolean, so I don't think it's a big deal, as long as we put a big note into the changelog.
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.
See all the broken update complaints?
What about introducing and checken for a new Interface? I think we need to switch thereto anyway in Users to be able to further extend it with optional capabilities.
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.
One approach would be to create a new interface (V2) with the new method, and make that interface extend this one. So "user backend v2" would implement the new interface.
Another possible alternative but possibly less popular is to use traits ? @icewind1991
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.
Accounts table ticket is here: #21282
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 that is 9.1 let's move that as well here?
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.
… or we we want to close this one completely?
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.
Seems there won't be an account table for 9.1 after all.
We should move forward with this or close it. No point in leaving a PR open for that long.
Moved to 9.0 as per #14127 (comment) |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
fixes #14127
please review @PVince81 @MorrisJobke