Add a command example

[codemasher]

Hey, here's a simple example that shows how slash commands are registered and how autocomplete on options works.

[codemasher]

I just noticed that the $params instance is empty here - shouldn't it contain the options that I'm fishing from $interaction->data?

This is what I'm getting

object(Discord\Helpers\Collection)#465 (1) {
  [""]=>
  object(Discord\Parts\Interactions\Request\Option)#187 (3) {
    ["attributes"]=>
    array(0) {
    }
    ["created"]=>
    bool(true)
    ["class"]=>
    string(41) "Discord\Parts\Interactions\Request\Option"
  }
}

[valzargaming]

It definitely should. I'll look into this more later tonight. I'm curious as to why the attributes array is empty because that is all of the data that should be sent from Discord, so the fact that it's empty means that there are no options being sent.

[codemasher]

Ok, I just had a glance over the unit tests, and tbh I'd start there before doing anything else: replace every instance of assertEquals with assertSame and you'll get fun things like this The option "token" with value false is expected to be of type "string", but is of type "bool". I'm not sure if this is exactly related to the issue right here, but you've been testing against broken unit tests all the time. Aside of that, I've seen way too many loose comparisons throughout the codebase, which are typically sources of error. I'd suggest updating the unit tests and add proper static analysis (phan, phpstan, php-codesniffer). You will cry blood and tears, but I promise you it's worth it, because hunting for bugs will be a lot easier.

Edit: Granted, I don't think the test error I cited above is caused by a loose comparison, but whatever caused that output could be replaced by a TestCase::assertSame() too.

Edit2: The mentioned error comes out of the Symfony options resolver (shudders).

[valzargaming]

We have unit tests that work, but I believe they were made to only function in the context of GitHub actions. I added dotenv to my local environment and updated DiscordTestCase as such and everything works as expected with the exception of testCanReplyToMessage:

public static function setUpBeforeClass(): void
{
    set_rejection_handler(function (\Throwable $e): void {});

    // Use vlucas/phpdotenv to load environment variables when running tests in a local environment
    $dotenv = Dotenv::createImmutable(dirname(__DIR__));
    if ($envVars = $dotenv->load()) {
        foreach ($envVars as $key => $value) {
            putenv("$key=$value");
        }
    }

    /** @var Channel $channel */
    $channel = wait(function (Discord $discord, $resolve) {
        $channel = $discord->getChannel(getenv('TEST_CHANNEL'));
        $resolve($channel);
    });
    self::$channel = $channel;
    assert(self::$channel instanceof Channel, 'Channel not found. Please check your environment variables and ensure TEST_CHANNEL is set.');
}

Tests: 91, Assertions: 184, Errors: 1, Risky: 2.

[codemasher]

By broken I mean primarily all the loose comparisons that will never reveal the real cause of an issue. PhpUnits's assertEquals (loose comparison) should never be used unless it's absolutely necessary.

This test for example does not what you think it does:

DiscordPHP/tests/CollectionsTest.php

Lines 164 to 167 in 01aae1f

	$this->assertEquals(true, $collection->has(1, 2, 3));
	$this->assertEquals(true, $collection->has(0));
	$this->assertEquals(false, $collection->has(5, 6, 7));
	$this->assertEquals(false, $collection->has(0, 5));

Aside, there's also assertTrue and assertFalse.

[codemasher]

You know you can just check $array === [] right? Then there's other cases where if(!empty()){...} is checkd and the result is handed over into a foreach without further check - in which case you should check for is_iterable() instead - in general, check for the expected types because in most cases empty() will just lead to something exploding.

[valzargaming]

I believe empty is also used to check for empty strings, and is meant to also be used where isset could also be checked beforehand. (e.g. if(! isset($var) || ! $var) could be shortened to just be if (! empty($var))`. Regardless, I would like to update these to just be basic if() statements and let the truth table logic attempt to see if it's equivalent to true or false. I need to look over each case to make sure there isn't any breaking change with doing so in some places, and figure out what is appropriate where. A lot of this is legacy code and will need to be refactored as part of library maintenance.

[codemasher]

I believe it's painstaking - I did the same in my most popular libraries too when there were a bunch of empty() related issues before.

[valzargaming]

Just wanted to quickly follow up on the topic of unit tests as this was on my mind somewhat today. We do have unit tests configured within our github workflow, but I do not believe they're currently being utilized (or contains a syntax error). The configuration is in .github/workflows/unit.yml

[codemasher]

I think this is better suited for another issue or even discussion rather than a comment on an already closed issue :)

[valzargaming]

If you run composer run-script cs or composer run-script cs-unsupported (if running php8.4) I will merge this

[codemasher]

I just tried and it reformatted everything except the example and uhh... lmao (give me a minute)

DiscordPHP/.php-cs-fixer.dist.php

Lines 99 to 101 in a9fbc42

	PhpCsFixer\Finder::create()
	->exclude('examples')
	->in(__DIR__)