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

Fix incorrect size check for MessageChannel#sendFile #1260

Merged
merged 1 commit into from
Apr 11, 2020
Merged

Fix incorrect size check for MessageChannel#sendFile #1260

merged 1 commit into from
Apr 11, 2020

Conversation

gpluscb
Copy link
Contributor

@gpluscb gpluscb commented Apr 11, 2020

Pull Request Etiquette

Changes

  • Internal code
  • Library interface (affecting end-user code)
  • Documentation
  • Other: _____

Closes Issue: NaN

Description

Fixes an issue where the byte[] overload of MessageChannel#sendFile
does not check permissions and ignores guild boosts in the max allowed size check.

@MinnDevelopment
Copy link
Member

For your info, it does check permissions since the sendFile(String, byte[]) just calls sendFile(String, InputStream) which does have an override.

@gpluscb
Copy link
Contributor Author

gpluscb commented Apr 11, 2020

Should this behaviour be changed?
The recently added TextChannelImpl/PrivateChannelImpl#sendFile(File, String, AttachmentOption...) methods have the same behaviour, so it would make sense to change these as well if this is going to be changed.

@MinnDevelopment
Copy link
Member

Well, checking permissions twice is kind of redundant. However, the code flow here is confusing, as you can probably tell. I'm fine either way, your call.

@gpluscb
Copy link
Contributor Author

gpluscb commented Apr 11, 2020

In that case I am going to leave it in for clarity sake.

@MinnDevelopment MinnDevelopment merged commit c0777ee into discord-jda:development Apr 11, 2020
@gpluscb gpluscb deleted the patch-file-limit branch April 11, 2020 23:19
@MinnDevelopment MinnDevelopment added this to the v4.2.0 milestone May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants