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.

Sign up for GitHub

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

Jump to bottom

MSC2582: Remove mimetype from EncryptedFile object #2582

Merged
merged 3 commits into from
Aug 30, 2021
Merged

MSC2582: Remove mimetype from EncryptedFile object #2582

merged 3 commits into from
Aug 30, 2021

Conversation

Sorunome
Copy link
Contributor

@Sorunome Sorunome commented May 26, 2020 •
edited by uhoreg
Loading

@Sorunome Sorunome mentioned this pull request May 26, 2020
Closed
@Sorunome
Copy link
Contributor Author

Related PR: #2581

@turt2live turt2live added blocked Something needs to be done before action can be taken on this PR/issue. proposal A matrix spec change proposal proposal-in-review labels May 26, 2020
@turt2live
Copy link
Member

Arbitrarily blocking this on someone commenting on #2581

@turt2live turt2live removed the blocked Something needs to be done before action can be taken on this PR/issue. label May 26, 2020
@turt2live
Copy link
Member

(per #matrix-spec: this PR is required to remove the attribute - unblocked).

@turt2live turt2live requested review from uhoreg and turt2live and removed request for uhoreg May 27, 2020 14:46
uhoreg
uhoreg reviewed May 27, 2020
View reviewed changes
proposals/2582-remove-mimetype-from-encrypted-file.md Outdated


## Potential issues
Some clients might depend on this?
Copy link
Member

@uhoreg uhoreg May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

TODO: check client support

(I will update this list as we determine what changes are needed for clients.)

(in alphabetical order)

  • fluffychat
  • gomuks
  • nheko
  • nio SDK
    • weechat-matrix (does not seem to use anything)
    • mirage
  • Riot Element Android
  • Riot Element iOS
  • Riot Element Web
    • the decryptFile function only gets passed the file property from the contents. To fix this, we'd need to either pass both the file and info properties, or the entire contents.
  • rust sdk (does not seem to do any mimetype processing on its own)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fluffychat doesn't seem to depend on this, either

Copy link
Member

Choose a reason for hiding this comment

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

Element Android dropped this field in element-hq/element-android#3273. Element iOS in matrix-org/matrix-ios-sdk#1125. The open issue for Element Web is element-hq/element-web#17145.

Copy link
Member

Choose a reason for hiding this comment

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

Element no longer uses this field, and uses the mimetype from the info field: matrix-org/matrix-react-sdk#6591 (It will still send the field for now, but that's less important for this MSC)

Copy link
Member

@uhoreg uhoreg Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@tulir do you know what gomuks does? Also, any of your bridges.

Copy link
Member

Choose a reason for hiding this comment

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

My libraries don't support that field at all

Copy link
Member

Choose a reason for hiding this comment

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

OK, so it seems like all clients don't use the undocumented field any more, so I think it this potential issue is no longer an issue.

@turt2live turt2live removed their request for review November 10, 2020 05:28
@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 8, 2021
@deepbluev7 deepbluev7 mentioned this pull request Apr 13, 2021
Closed
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
This was referenced Aug 6, 2021
Closed
Closed
Closed
uhoreg
uhoreg reviewed Aug 10, 2021
View reviewed changes
proposals/2582-remove-mimetype-from-encrypted-file.md

## Proposal
The example in the spec currently lists `mimetype` in the examples for `EncryptedFile` but not in
the object definition. As that is duplicate information of the `info` block of file events, the
Copy link
Member

Choose a reason for hiding this comment

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

Just to note that the EncryptedFile appears twice in the examples: in the file property (top-level) and it info.thumbnail_file. mimetype is given in both places.

@uhoreg uhoreg mentioned this pull request Aug 10, 2021
Merged
uhoreg added a commit to matrix-org/matrix-react-sdk that referenced this pull request Aug 10, 2021
@uhoreg
use the mimetype from the info property rather than the EncryptedFile
3b98107 
the mimetype in EncryptedFile is undocumented and redundant.
see matrix-org/matrix-spec-proposals#2582
@turt2live turt2live self-requested a review August 11, 2021 13:43
@uhoreg uhoreg removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Aug 11, 2021
@uhoreg
Copy link
Member

uhoreg commented Aug 11, 2021

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Aug 11, 2021 •
edited by erikjohnston
Loading

Team member @uhoreg 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.

@mscbot
Copy link
Collaborator

mscbot commented Aug 24, 2021

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

@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 Aug 24, 2021
@Sorunome
Copy link
Contributor Author

Just committed the two suggestions 👍

@mscbot
Copy link
Collaborator

mscbot commented Aug 29, 2021

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

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Aug 29, 2021
@turt2live turt2live merged commit e895827 into matrix-org:old_master Aug 30, 2021
turt2live pushed a commit that referenced this pull request Aug 30, 2021
@Sorunome @uhoreg @turt2live
MSC2582: Remove mimetype from EncryptedFile object (#2582)
d9dd37d 
* add proposal

* Update proposals/2582-remove-mimetype-from-encrypted-file.md

Co-authored-by: Hubert Chathi <[email protected]>

* Update proposals/2582-remove-mimetype-from-encrypted-file.md

Co-authored-by: Hubert Chathi <[email protected]>

Co-authored-by: Hubert Chathi <[email protected]>
@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 finished-final-comment-period labels Aug 30, 2021
afranke pushed a commit to afranke/matrix-doc that referenced this pull request Sep 1, 2021
@Sorunome @uhoreg @afranke
MSC2582: Remove mimetype from EncryptedFile object (matrix-org#2582)
d9815af 
* add proposal

* Update proposals/2582-remove-mimetype-from-encrypted-file.md

Co-authored-by: Hubert Chathi <[email protected]>

* Update proposals/2582-remove-mimetype-from-encrypted-file.md

Co-authored-by: Hubert Chathi <[email protected]>

Co-authored-by: Hubert Chathi <[email protected]>
turt2live added a commit that referenced this pull request Sep 24, 2021
@turt2live
Remove extraneous mimetype from EncryptedFile examples, per MSC2582
bcf8315 
MSC: #2582
@turt2live turt2live mentioned this pull request Sep 24, 2021
Merged
@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 Sep 24, 2021
@turt2live turt2live self-assigned this Sep 24, 2021
@turt2live
Copy link
Member

Spec PR: #3412

turt2live added a commit that referenced this pull request Sep 27, 2021
@turt2live
Remove extraneous mimetype from EncryptedFile examples, per MSC2582 (#…
b7e6104 
…3412)

* Remove extraneous mimetype from EncryptedFile examples, per MSC2582

MSC: #2582

* changelog
@turt2live
Copy link
Member

Merged 🎉

@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 Sep 27, 2021
@uhoreg uhoreg mentioned this pull request Jul 26, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tulir tulir tulir left review comments

@richvdh richvdh richvdh left review comments

@uhoreg uhoreg uhoreg left review comments

@anoadragon453 anoadragon453 anoadragon453 approved these changes

@turt2live turt2live turt2live approved these changes

Assignees

@turt2live turt2live

Labels
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
Spec Core Team Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Sorunome @turt2live @uhoreg @mscbot @anoadragon453 @richvdh @tulir