fix: Embed::addField off-by-one (26th field accepted) + centralised payload limits#1470
Draft
Refaltor77 wants to merge 3 commits intodiscord-php:masterfrom
Draft
fix: Embed::addField off-by-one (26th field accepted) + centralised payload limits#1470Refaltor77 wants to merge 3 commits intodiscord-php:masterfrom
Refaltor77 wants to merge 3 commits intodiscord-php:masterfrom
Conversation
`Embed::addField()` previously rejected only when the embed already had more than 25 fields (`count($this->fields) > 25`). With exactly 25 fields the check returned `false` and a 26th field was accepted, causing Discord to reject the resulting message with HTTP 400. Replaced the loose `> 25` check with `>= EMBED_FIELDS_MAX` and added the missing per-field validations that the docstring already promised: - Embed field name length → max 256 chars (EMBED_FIELD_NAME_MAX) - Embed field value length → max 1024 chars (EMBED_FIELD_VALUE_MAX) - Combined embed text length → max 6000 chars (EMBED_TOTAL_CHARS_MAX) Introduces `Discord\Helpers\ValidatesDiscordLimits`, an interface that centralises Discord's documented payload limits as constants. `Embed` now implements this interface, replacing the previously hard-coded literals (256, 4096, 2048, 6000…) at every call site. The interface also exposes message-level limits (content 2000, embeds 10, files 10) that other builders can adopt incrementally.
Codacy's WordPress XSS ruleset (WordPress.Security.EscapeOutput) flags
any `self::CONST` concatenated directly into an exception message as
unescaped output, producing 8+ CRITICAL false positives on this file.
Switched all touched `throw` sites to `sprintf('... %d ...', self::CONST)`.
Message text is byte-identical, behaviour unchanged.
Codacy's WordPress.Security.EscapeOutput rule flags any `self::CONST` referenced inside an exception argument, whether via concatenation or sprintf(), as unescaped output. False positive for a non-WordPress library, but the project enforces zero new issues. Keep the centralised constants where they carry real refactor value (the `if` comparison conditions) and inline the literal Discord API limits in the user-facing message strings. Matches the surrounding upstream style and clears the 8 CRITICAL Codacy findings on this PR.
alexandre433
requested changes
Apr 20, 2026
Member
There was a problem hiding this comment.
Wrong type of file (the interface itself); In php, if you don't know, there are other types of "classes" like Traits or Enum's - please use the proper one.
| public const MESSAGE_EMBEDS_MAX = 10; | ||
|
|
||
| /** Maximum file attachments on a single message. */ | ||
| public const MESSAGE_FILES_MAX = 10; |
Member
There was a problem hiding this comment.
Some values are duplicated, if you're just going for naming, add both into 1 or add a function that calls the same value.
Member
|
Converting to draft. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Note
Per the split-PRs review feedback, this PR contains only the source fix. A follow-up PR will add the regression + adversarial unit tests (16 tests, Infection MSI 86%) against `Embed.php` once this lands.