Skip to content

[Notifier] [Discord] Fix value limits #60269

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

Merged
merged 1 commit into from
Apr 27, 2025

Conversation

norkunas
Copy link
Contributor

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues N/A
License MIT

It was not properly calculating values length with non latin characters for embed data. This makes it consistent with calculating chat message length in DiscordTransport.

@norkunas norkunas requested a review from OskarStark as a code owner April 25, 2025 09:29
@carsonbot carsonbot added this to the 6.4 milestone Apr 25, 2025
@OskarStark OskarStark changed the title [Notifier] [Discord] Fix value limits [Notifier][Discord] Fix value limits Apr 25, 2025
@OskarStark
Copy link
Contributor

Hmm, what about removing the client side validation completely and let the API return a proper error? Can you check what is happening if we would do so?
My point is, that we cannot keep track of all new and updated limitations in the future and we always need a fix PR + a release 🤷‍♂️

@carsonbot carsonbot changed the title [Notifier][Discord] Fix value limits [Notifier] [Discord] Fix value limits Apr 25, 2025
@norkunas
Copy link
Contributor Author

I'm pretty sure that limits were not changed, just wrong calculation was used :) I'm fine either way. But at least for me it's easier when i can click on the method like addField and see what are the limits, instead of going to discord docs.

Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Ok then, lets do it

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

I'm going to merge it as a bug fix, but Oskar is right, we avoid doing the validation on our side as it forces us to make sure we update the code whenever upstream changes as well.

@fabpot
Copy link
Member

fabpot commented Apr 27, 2025

Thank you @norkunas.

@fabpot fabpot merged commit 06cccc5 into symfony:6.4 Apr 27, 2025
11 checks passed
@norkunas norkunas deleted the fix-discord-value-limits branch April 27, 2025 15:50
This was referenced May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants