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
MSC4133: Extending User Profile API with Key:Value Pairs #4133
base: main
Are you sure you want to change the base?
MSC4133: Extending User Profile API with Key:Value Pairs #4133
Changes from 1 commit
0e85b3b
0f373db
c5a3015
c8a5a1a
3157982
b8ed87a
a81f21f
e688eb1
39d5fa2
e160c71
a9546aa
0c43f50
613411a
4a9557f
fbb4e44
591999f
15a0bd1
26c59f7
30e82aa
2966c85
d97189e
ae19725
23a3a62
f32932c
4afb8b8
bb4fc76
09318e8
9b4741e
068d44e
fa381da
b373a55
3349123
9fa0096
da0a791
a948f5d
c82dab6
21ad83f
1726ef3
30d203e
43e1e1a
af87bbe
f686090
17cc30b
d620259
c1b419a
4b3a8ed
0f41c82
1b98d40
9b2918e
e00d2e9
a11286a
2a86235
c4fa474
7ca83db
c7bb382
3843f27
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.
Moving to a thread for discussion to take place - @MTRNord said:
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'm failing to understand how this MSC creates problems for pronouns specifically. This MSC defines a generic key/value structure, where the value can be pretty much any type. This could be a user-supplied string, an object, or a boolean value depending on the key's requirements. If you have specific concerns with how the MSC is interfering with other MSCs, like pronouns, please leave inline comments for review.
This MSC also states it complements profiles as rooms, which is true: one of the concerns with profiles as rooms is it tries to do too much. By accepting this MSC, we're agreeing that arbitrary key/value pairs should be supported, which makes other MSCs easier to land. In essence, this is already a continuation of the work, just at an earlier stage than the current MSCs in the area. Forking the effort feels incorrect, given all the authors involved would like to work with each other towards a solution. If there's specific language which doesn't feel in line with this, please leave an inline comment for review.
As for room moderation: profile fields are deliberately kept at the user level in this MSC. A future MSC may introduce them to the room/member level as well. This makes the T&S model one where the server admin is responsible for the user's profile, not the rooms they are participating in. Room admins still have the option to eject users they don't feel fit their room's purpose, and can operate bots which scan profiles for potentially abusive content if they wish. Meanwhile, abusive users and their profiles should be reported to the homeserver admin, which this MSC calls out as the preferred mechanism for dealing with issues. A future, unrelated, MSC will make this easier. If there's specific areas which could be improved further, inline comments are appreciated for further review.
Creating an MSC for every feature may be what happens at the start of the transition towards this key/value system, but as we gain experience in reviewing these MSCs we'll be better able to calibrate the types of things which should be in the spec and which are best covered as unstable extensions. For example, we may prefer a general
m.bio
field over lots of fields for gender, websites, contact info, and interests. Or we may prefer to have a dedicated field for gender, and combine the rest underm.bio
. These types of concerns are best expressed on the individual MSCs which get opened after this MSC lands - the influx is expected, and something we'd need to manage regardless.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 am currently on the move and cant draft a full reply in the meanwhile many of my concerns also have been raised by gnuxie at #4133 (comment) which likely answer some of the questions. I will write a proper reply once i am back at a computer :)
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.
You mean #4201? Which exists just to give PaR another try.
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.
@Gnuxie wrote:
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.
Whilst we do make an effort to avoid speccing stuff that doesn't have a demonstrated purpose, there is a balance to make between that, and allowing things to land in manageable pieces. Historically, some features have struggled to make it through the spec process because their scope was just too broad. (MSC1849 was an example of that.)
Reviewing large MSCs is really hard (as is reviewing a large PR of any kind), just because everybody has to keep lots of context in their head for weeks or even months, and breaking them up helps build stable foundations before moving onto the next storey.
Further: in the case of this particular MSC, I see it mostly as a generalisation of the existing API. Even if we never end up landing anything like MSC4175, it doesn't feel like this MSC is adding undue clutter to the API.
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.
@Gnuxie wrote:
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 don't think I really understand the concern here. It feels like you're arguing against this MSC because you're concerned that people may, in the future, use it as a basis for other MSCs which bring problems. You've acknowledged yourself that such problems are not inherent by pointing to MSC4175, so it seems inappropriate to express your concerns on this MSC. If you have concerns with MSC4208, then they belong on MSC4208, not here.
Yeeess, but then the time to discuss them is on those MSCs, not here.
To be clear, I'm reading this as: "clients MAY provide a UI for users to view and enter their own profile fields". On that basis, it's hard to see why this, on the face of it, is any different to, say, allowing users to send freetext events.
I must admit I'm not that familiar with the T&S space, so it's entirely possible I'm missing something here, but in discussions I've had with people who are more familiar with it than me, they've not been concerned about letting users set profile data: it's when clients start showing the profile data of other users that you need to be concerned. I'd be happy to be set straight 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.
@Gnuxie wrote:
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.
Firstly, I'm sorry if your concerns haven't been heard. I don't think I've seen the discussions you refer to, and I wasn't aware of them before you raised them here. This is one reason we ask that concerns on MSCs be raised as comments on the MSC itself: conclusions in Matrix rooms get lost. Thank you for doing so now.
Secondly: I don't really see this as "urgency". The MSC has been 9 months in the making, and it feels like a relatively incremental change to the existing API. Honestly, it felt ready to land. I'll refer you to my earlier points that it can be very helpful to land MSCs incrementally rather than having a large amount to review all at once.
As the SCT, we've also had feedback in the past that it can be unreasonably difficult to get simple changes to the spec through the MSC process. There are numerous anecdotes from people who have just found the MSC process too arduous and have walked away in frustration. We have a balance to pick between rushing things into the protocol without due diligence, and letting the protocol ossify because changing it is just to damn hard.
All that said, you're right that this isn't actively blocking anything, and it's certainly not our goal to steamroller it through against significant opposition. On that basis: thank you again for raising your concerns. This is the FCP working as it should: we've put the FCP timer at least on hold for now and we'll see what can be done to address the concerns.
Whilst we can't promise that everyone is going to be delighted by every MSC that is accepted, it is important to make sure that members of the Matrix community have the opportunity to voice their concerns and that we take the time to consider those concerns properly.