-
Notifications
You must be signed in to change notification settings - Fork 384
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
MSC3567: Allow requesting events from the start/end of the room history #3567
Conversation
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.
Overall looks great - just a point of clarity before chucking this through the process.
This seems relatively straightforward, and has a concrete use case. @mscbot fcp merge |
Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people: 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. |
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 not saying "no", but I've yet to be convinced that this is the best approach.
Honestly, it sounds like a case of "we've implemented this in a way that violates the spec, and now we want to change the spec make it legal".
[a better justification, tbh, is "synapse does it, and it's pointless to try to hold back that tide now"]
Just want to add that this would help us avoid some serious gymnastics we perform in Mjolnir before redacting a user's events https://github.com/matrix-org/mjolnir/blob/main/src/utils.ts#L101-L169 |
ok, I've been convinced! |
Following new guidance from matrix-org/matrix-spec-proposals#3567
@richvdh can I further convince you to check an FCP box? 😇 |
@@ -0,0 +1,67 @@ | |||
# MSC3567: Allow requesting events from the start/end of the room history |
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.
Is there a reason why synapse supports pagination from the start of the room history?
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
Spec PR: #3638 |
As per MSC3567, the `from` parameter is now optional for the `/messages` endpoint to allow fetching first or latest room content without having to rely on `/sync` matrix-org/matrix-spec-proposals#3567
As per MSC3567, the `from` parameter is now optional for the `/messages` endpoint to allow fetching first or latest room content without having to rely on `/sync` matrix-org/matrix-spec-proposals#3567
The new spec PR: matrix-org/matrix-spec#1002. The spec PR was merged so I think the labels need an update here! |
Rendered
Fixes #1892
Implementation in Synapse: https://github.com/matrix-org/synapse/blob/b1ecd19c5d19815b69e425d80f442bf2877cab76/synapse/handlers/pagination.py#L438-L441
Preview: https://pr3567--matrix-org-previews.netlify.app