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

🚧 Initial Discord message components v2 implementation #1294

Draft
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

Log1x
Copy link
Member

@Log1x Log1x commented Feb 21, 2025

🚧 Work in progress.

Completely blind & untested. Should be able to try it later today.

Edit: everything appears to be functional.

Screenshot

@valzargaming
Copy link
Member

The v2 contract was an excellent idea, and something I'd like to see in other parts of the library. It would certainly cut down on a lost of instances of checking against hardcoded types like this.

Copy link
Member

@SQKo SQKo left a comment

Choose a reason for hiding this comment

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

Please also adjust the class implements Components\xx to have the namespace imported per my previous suggestion

public function setUrl(string $url): self
{
$this->url = $url;

Copy link
Member

Choose a reason for hiding this comment

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

Implement validation of $url like in Embed::ensureValidUrl()

Copy link
Member

Choose a reason for hiding this comment

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

Do we know if it's the same spec? We could just copy-paste the function here and do the same thing.

Copy link
Member

Choose a reason for hiding this comment

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

I've confirmed it's exactly the same except for File, in which case only attachment is allowed.

public function addComponent(Component $component): self
{
if (! ($component instanceof TextDisplay)) {
throw new \InvalidArgumentException('Section can only contain TextDisplay components. Use setAccessory() for Thumbnail or Button.');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new \InvalidArgumentException('Section can only contain TextDisplay components. Use setAccessory() for Thumbnail or Button.');
throw new \InvalidArgumentException('Section can only contain TextDisplay components.');

You are not checking wether the $component is a Thumbnail or Button here so it's better not to explicitly say that.

Copy link
Member

Choose a reason for hiding this comment

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

On a second though, the spec is telling us to not hardcode this, so we should not validate this yet

Suggested change
throw new \InvalidArgumentException('Section can only contain TextDisplay components. Use setAccessory() for Thumbnail or Button.');
trigger_error('Section may only contain TextDisplay components.', E_USER_NOTICE);

Copy link
Member

Choose a reason for hiding this comment

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

I’m not comfortable introducing a change like trigger_error in this PR as part of new functionality. We have always made it a point to ensure that new features align with the library’s existing design, and currently, the only places where trigger_error is used are in __construct methods related to the initial creation of the Discord class and its associated CacheInterface. Making this change here without a broader review of the library to assess its justification elsewhere would be premature. If we decide to introduce this pattern, it should be considered separately from this PR.

Copy link
Member

@SQKo SQKo Mar 1, 2025

Choose a reason for hiding this comment

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

Then don't throw the errors, remove the argument checking, and leave the type Component in the parameter while changing the phpdoc @param TextDisplay for now

Comment on lines 83 to 85
public function setAccessory(Thumbnail|Button $component): self
{
$this->accessory = $component;
Copy link
Member

@SQKo SQKo Feb 23, 2025

Choose a reason for hiding this comment

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

The spec is telling us to not hard code the supported components yet (anyway leave the phpdoc as is)

Suggested change
public function setAccessory(Thumbnail|Button $component): self
{
$this->accessory = $component;
public function setAccessory(Component $component): self
{
if (! ($component instanceof Thumbnail || $component instanceof Button)) {
throw new \InvalidArgumentException('Accessory may only contain Thumbnail or Button component.');
}
$this->accessory = $component;

Copy link
Member

Choose a reason for hiding this comment

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

I agree with this, with the condition that throw new \InvalidArgumentException should be used instead for now.

Copy link
Member

Choose a reason for hiding this comment

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

No, throwing error is the same as hardcoding it, the spec tells us NOT to

if you insist users should be informed then go back with my previous suggestions of trigger_error() as the E_USER_NOTICE doest not throw error but showing notice tracing to this code in their logs.

Copy link
Member

Choose a reason for hiding this comment

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

The rest of the library does not do this. It should be a separate PR.

@SQKo
Copy link
Member

SQKo commented Feb 23, 2025

According to the spec, using the components v2 may disable content and embeds, it would be good to warn the users in the MessageBuilder to avoid confusion

Also seems this PR has not supported webhooks yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants