Feedback on "Tightening down php code sniffer" (#223)

[jaspernbrouwer]

In order to move forward, I've made some changes to PR #223 and merged it, but some things need to be discussed. I'll list them here, so we can decide what to do.

Redundend annotations

/**
  * We don't use associations, so we can safely store the user in sessions.
  * The user-provider will refresh the user, to make it complete and managed.
  *
  * @return string
  */
public function serialize(): string

Same here.. If PHPStorm could just stop complaining about missing return values that are declared in the code, that would be great. Mkay.

Indeed, PhpStorm's inspections require these annotations when a docblock is present. There is an option that allows you to omit @param and @return when type-hints are in place, but they behave inconsistently.

Personally I'm in favor of having them and consistent, over not having them with weird exceptions. Plus I really like being able to "Inspect code" and having a clean list of what I need to fix.

mixed[]

/**
  * @return mixed[]
  */
public static function getSubscribedEvents(): array

Yuck. Can't we point at a parent docblock with @inherited instead? :D

This is regarding the rule SlevomatCodingStandard.TypeHints.TypeHintDeclaration.MissingTraversableReturnTypeHintSpecification, which states that you should be more specific that array (or Traversable, etc). I like this rule, because in most cases you can be more specific (string[] instead of array for example).

In this case, we cannot be more specific, because several array-structures are allowed.

@return mixed[] is kind of a workaround, mixed is deemed specific enough. The alternative is suppressing the rule, but I want to minimize that.

PS: I'm not sure if phpcs looks at the parent in these cases. In this specific case the parent is Symfony\Component\EventDispatcher\EventSubscriberInterface, isn't more specific that @return array, so there is no "correct" parent to fall back to.

asd

public static function availableTypes(): array
{
    try {
        $reflected = new ReflectionClass(self::class);
    } catch (ReflectionException $e) {
        return [];
    }

What exactly are we protecting ourselfves from here? This try-catch will never ever catch anything.

(by the way, I think the basic problem with this code is me writing crap - I should just have made static constructors and not bothered with the constants)

I must confess that I'm not sure why I changed this. But I think it's because I'd prefer the try/catch over suppressing an inspection.

!$var vs empty($var)

return !$diff && count($this->breeds) === count($otherCollection->breeds);

Since array_diff returns an array, what is wront with empty()? Just curious..

empty($diff) has the same behavior as !isset($diff) || !$diff (or isset($diff) === false || $diff == false). Since we know isset($diff) will return true, because $diff is defined in the line before the if, that !isset($diff) part is irrelevant. What remains then, is !$diff.

I can follow that $diff is an array (or list), and we want to test "is the list empty?", then if (empty($list)) makes sense. But IMHO we're not speaking English here, but we're speaking PHP. So, if we really want to make "is the diff empty?" explicit in PHP, it would look something like count($diff) === 0. I think it's important to speak English when naming things (variables, methods, etc), but the actual implementation remains PHP.

Personally if ($var) or if (!$var) (i.e. does it evaluate to true or false) is fine. I don't really need to go as far as if ($var === true) or if ($var === false) (or count($diff) === 0). But hey! It's a matter of taste / personal preference, so I'm open to alternatives 🙂

Return early

foreach ($this->elePHPants as $key => $elePHPant) {
    if (!$elePHPant->elePHPantId()->equals($elePHPantId)) {
        continue;
    }

    unset($this->elePHPants[$key]);
}

I don't quite agree with the return early here, since we loop over all the elements anyways.

foreach() { if (true) {do something }}
is more comprehensible than
foreach() { if (!true) { continue } do something}

But it's also not a big thing.

True I followed the rule:

Return early, so:

• the rest is 1 indentation less
• keep the main goal on the main indentation and have the exceptions move 1indendation higher (instead of the other way around).

I do agree that in this specific case the benefits are exrtemely small 🙂

Indentation in heredocs

We can use indentation in heredocs now!

$dql = <<<'EOQ'
           SELECT COUNT(e)
           FROM Herding:ElePHPant e
           WHERE e.adoptedOn BETWEEN :from AND :to
           EOQ;

Should we add that? (not in this PR, just a remark)

Personally I like them without indentation. I usually use them when the content gets larger (above 2 or 3 lines), so make use of the full width as well.
But this opinion isn't that strong, so I'm fine either way!

@phpcsSuppress

I've seen this a few times now and I'm triggered every time. I've decided I'm not a big fan of polluting the codebase with annotations meant for test-tooling... is there any way we can put these definitions outside the code?

Yes, these suppressions can be done in the phpcs.xml file itself. For example:

<rule ref="PSR1.Methods.CamelCapsMethodName">
    <exclude-pattern>/(specs|tests)/</exclude-pattern>
</rule>

On one hand, I too dislike @phpcsSuppress (and @noinspection) in the code, but I dislike having to maintain them elsewhere even more.
Mainly because these suppressions tend to be added, but never removed. In my experience, nobody actually bothers to look at them and check if they're still needed.
So I prefer having them in the code where they tend to be maintained, over having them in phpcs.xml where they are forgotten.

Less strict in tests

public function test_command_adopt_elephpant_returns_http_status_202(): void

I really don't feel like having to "properly" code all the test functions. Tests do not require the same stricness as production code in my opinion.

I'm all for being less strict in test-classes. Let's make a list of rules that can be suppressed for these!