-
Notifications
You must be signed in to change notification settings - Fork 386
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
MSC4213: Remove server_name parameter from join
and knock
endpoints
#4213
base: main
Are you sure you want to change the base?
Conversation
b894e89
to
a083ae4
Compare
Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
a083ae4
to
f011a86
Compare
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.
Implementation requirements:
I'm tempted to mirror #4127 (comment) and say "none", but am second-guessing that MSC's requirements too. It might be beneficial to have evidence that developers are picking up the newer approaches before suggesting removal from the spec.
Thoughts very welcome.
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 might not count because they were all created by me but there are five implementations linked in the proposal's body, four of which have already landed.
Using the FCP mechanism to encourage comment/explicit blockers: @mscbot fcp merge |
Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people: Concerns:
Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
@mscbot concern implementation-needs-checking label |
|
||
- synapse: https://github.com/element-hq/synapse/pull/17650 | ||
- dendrite: https://github.com/matrix-org/dendrite/pull/3438 | ||
- matrix-js-sdk: https://github.com/matrix-org/matrix-js-sdk/pull/4381 |
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's possible I'm confused but matrix-org/matrix-js-sdk#4381 removes the unstable prefix from the 'via' param. This MSC is about removing the old server_name param, which is a different thing.
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.
The linked implementations are for MSC4156 which added the via
param. They're listed to show that the old server_name
parameter is safe to remove
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.
Yes, exactly. I think the server_name
parameter will probably stay in implementations for quite a while unless they consciously choose to only support Matrix >= 1.12. But we don't need it in the spec anymore.
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.
Oh right, I see - I guess I was confused by your comment. In that case I guess I can vouch for the js-sdk impl, in that it now sends the via param.
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.
The server impl looks plausible too. I think we could remove the implementation-needs-checking
label.?
join
and knock
endpoints
@mscbot resolve implementation-needs-checking label |
Rendered
In line with matrix-org/matrix-spec#1700, the following disclosure applies:
I am a Systems Architect at gematik, Software Engineer at Unomed, Matrix community member and former Element employee. This proposal was written and published with my community member hat on.
FCP tickyboxes