[Notifier] Check Discord embed limitations #47859
Conversation
995c655 to
fa4a60c
Compare
|
@alamirault thanks for this contribution. I said the following before and folks didn't think it was an issue, but let me tell you again in case we can reconsider this: (1) I think it's a mistake to add Discord (or any other third-party) validation logic in Symfony code. The reason is that Discord can change these rules at any time, making Symfony code outdated. (e.g. in this PR we validate that (2) We should fix/report obvious errors (e.g. you pass a string where a boolean is required) but we should not try to over-validate things. For example, if today's Discord field limit is |
| @@ -111,6 +125,10 @@ public function author(DiscordAuthorEmbedObject $author): static | |||
| */ | |||
| public function addField(DiscordFieldEmbedObject $field): static | |||
| { | |||
| if (self::FIELDS_LIMIT === \count($this->options['fields'] ?? [])) { | |||
| throw new LengthException(sprintf('Maximum number of fields should not exceed %d.', self::FIELDS_LIMIT)); | |||
There was a problem hiding this comment.
| throw new LengthException(sprintf('Maximum number of fields should not exceed %d.', self::FIELDS_LIMIT)); | |
| throw new LengthException(sprintf('Maximum number of fields must not exceed %d.', self::FIELDS_LIMIT)); |
|
I agree with @javiereguiluz and I think we should instead make the error message better if possible (the current one is indeed not helpful: |
|
@javiereguiluz I understand, it's logic ! However, what do you think about adding a warning that checks are not all covered by Symfony on component's README ? And add link to official API docs ? |
|
I don't think there is a need for additional docs or links; the error messages coming from the API should be enough. |
|
Thank you @alamirault. |
|
I'm glad the issue got resolved one way or another, thanks @alamirault 👍 However, I'm still saddened that there's no useful documentation regarding these notifiers besides the generic documentation. I reckon adding just 1 complete example for each notifier in its own repo would already be a huge improvement, especially for those who have no idea how to customise the chat messages. |
|
I would appreciate a sub-section per notifier with some in-depth examples 👍 |
Add missing check on Discord Embeds (length and number).
See #47688 for description
Official embed limits: https://discord.com/developers/docs/resources/channel#embed-object-embed-limits
(unrelated phpunit failure)