Ignore permalink_prefix when serializing pills #11726
Conversation
github-actions
bot
added
the
Z-Community-PR
| public forEvent(roomId: string, eventId: string, serverCandidates: string[]): string { | ||
| public forEvent(roomId: string, eventId: string, serverCandidates: string[], ispill = false): string { | ||
| if (ispill) { | ||
| return `https://matrix.to/#/${roomId}/${eventId}${this.encodeServerCandidates(serverCandidates)}`; |
There was a problem hiding this comment.
This syntax is already generated by MatrixToPermalinkConstructor. I think probably the right fix here would be to pass the flag into makeGenericPermalink but then have getPermalinkConstructor() return the matrix.to one instead. This should make this change a lot simpler as all the various methods of the different PermalinkConstructor classes won't need the extra param.
src/utils/permalinks/Permalinks.ts
Outdated
| @@ -269,26 +269,26 @@ export class RoomPermalinkCreator { | |||
| }; | |||
| } | |||
|
|
|||
| export function makeGenericPermalink(entityId: string): string { | |||
| return getPermalinkConstructor().forEntity(entityId); | |||
| export function makeGenericPermalink(entityId: string, ispill = false): string { | |||
There was a problem hiding this comment.
Could the param follow the code style & be camelCased to isPill please? I think I read it asm, "I spill" the first time, and that's in the context of this PR.
Also this function could really benefit from some doc now it's not just a simple single-parameter function, as it's not really obvious how the isPill parameter changes the function's behaviour.
There was a problem hiding this comment.
Where/How should I document that? A separate file in docs/ seems a bit overkill...
Just as comments in the source?
There was a problem hiding this comment.
Yeah, a jsdoc comment is perfect (eg.
matrix-react-sdk/src/utils/permalinks/Permalinks.ts
Lines 305 to 311 in a324024
fixes element-hq/element-web/issues/26002 During serialization of messages, pills were wrongfully serialized to a URL starting with `permalink_prefix`. This is against the Spec (which mandates https://matrix.to/#/ links) and the resulting pills were not recognized as pills in other clients. Spec-Appendix concerning matrix.to links: https://spec.matrix.org/v1.8/appendices/#matrixto-navigation Signed-off-by: Lars Wickel <git@herkulessi.de>
Signed-off-by: Lars Wickel <git@herkulessi.de>
Signed-off-by: Lars Wickel <git@herkulessi.de>
Signed-off-by: Lars Wickel <git@herkulessi.de>
Signed-off-by: Lars Wickel <git@herkulessi.de>
herkulessi
force-pushed
the
fix-premalink_prefix-pills
branch
from
bbe4618 to
ba11663
Compare
dbkr
added
the
T-Defect
Checklist
fixes element-hq/element-web/issues/26002
During serialization of messages, pills were wrongfully serialized to a URL
starting with
permalink_prefix. This is against the Spec (which mandateshttps://matrix.to/#/ links) and the resulting pills were not recognized as
pills in other clients.
Spec-Appendix concerning matrix.to links: https://spec.matrix.org/v1.8/appendices/#matrixto-navigation
Signed-off-by: Lars Wickel git@herkulessi.de
This PR currently has none of the required changelog labels.
A reviewer can add one of:
T-Deprecation,T-Enhancement,T-Defect,T-Taskto indicate what type of change this is, or addType: [enhancement/defect/task]to the description and I'll add them for you.