Skip to content
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

MSC1704: Adding ?via= to matrix.to permalinks to help with routing #1704

Merged
merged 1 commit into from
Jan 10, 2019

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Oct 26, 2018

@turt2live turt2live added proposal-ready-for-review proposal A matrix spec change proposal proposal-pr client-server Client-Server API labels Oct 26, 2018
@turt2live turt2live changed the title Proposal to add ?via to matrix.to permalinks MSC 1704: Adding ?via= to matrix.to permalinks to help with routing Oct 26, 2018
https://matrix.to/#/!somewhere:example.org/$something:example.org?via=example-1.org&via=example-2.org
```

Clients can pass the servers directly to `/join` in the form of `server_name`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are these server_name parameters of which you speak?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, i misread /join as the slash-command rather than the API.

parameters.

When generating the permalinks, clients should pick servers that have a reasonably
high chance of being in the room in the distant future. The current recommendation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we try to use the server from the domain part of the room id?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo servers should already be appending the server name in the room ID to the candidates so clients don't have to worry about it. There's no guarantee that the server name in the room ID is still a resident in the room, or has a chance of being around for a while so I'm further hesitant to recommend it.

@turt2live
Copy link
Member Author

There hasn't been a whole lot of feedback in the negative for this proposal, and I think I've addressed the concerns that were raised:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Nov 26, 2018

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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 info about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Nov 26, 2018
@richvdh richvdh changed the title MSC 1704: Adding ?via= to matrix.to permalinks to help with routing MSC1704: Adding ?via= to matrix.to permalinks to help with routing Dec 12, 2018
@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Jan 5, 2019
@mscbot
Copy link
Collaborator

mscbot commented Jan 5, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

By adding a new parameter to the end, receivers can more easily join the room:

```
https://matrix.to/#/!somewhere:example.org?via=example-1.org&via=example-2.org
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is a breaking change for any clients that parsed matrix.to urls using naive string-splitting. Per @turt2live in #matrix-spec:matrix.org, the clients he sampled didn't fall into this category and either handled matrix.to links like real URIs or didn't handle them at all. I don't see it as a huge issue (especially not compared to how awful including matrix.to in the spec at all is), but I thought it was worth documenting this concern on the PR.

@mscbot
Copy link
Collaborator

mscbot commented Jan 10, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot removed the final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. label Jan 10, 2019
@turt2live turt2live merged commit 576aa22 into master Jan 10, 2019
@turt2live turt2live deleted the travis/msc/matrix.to-permalinks branch January 10, 2019 18:22
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed disposition-merge finished-final-comment-period labels Jan 10, 2019
@turt2live turt2live self-assigned this Apr 5, 2019
turt2live added a commit that referenced this pull request Apr 5, 2019
Spec for [MSC1704](#1704)

Reference implementations:
* Original: matrix-org/matrix-react-sdk#2250
* Modern recommendations: https://github.com/matrix-org/matrix-react-sdk/blob/2ca281f6b7b735bc96945370584c5cb1b5f7e1f1/src/matrix-to.js#L29-L70

The only deviation from the original MSC is the recommendation for which servers to pick. The original MSC failed to consider server ACLs and IP addresses correctly, and during implementation it was realized that both of these cases should be handled. The core principles of the original MSC are left unaltered.
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Apr 5, 2019
@turt2live
Copy link
Member Author

Merged via #1955 🎉

@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Apr 8, 2019
@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec and removed proposal-pr labels Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants