Enable phpstan

[ildyria]

Just to be nice...

• The workflow is disabled so the code still passes CI.
• Once merged in master this will allow to then merge in WIP branches and hopefully decrease the conflicts.

In order to run it just do:
make phpstan (and cry)

The rules so far are:

1. basic checks, unknown classes, unknown functions, unknown methods called on $this, wrong number of arguments passed to those methods and functions, always undefined variables
2. possibly undefined variables, unknown magic methods and properties on classes with __call and __get
3. unknown methods checked on all expressions (not just $this), validating PHPDocs
   return types, types assigned to properties
4. basic dead code checking - always false instanceof and other type checks, dead else branches, unreachable code after return; etc.
5. checking types of arguments passed to methods and functions
6. report missing typehints
7. report partially wrong union types - if you call a method that only exists on some types in a union type, level 7 starts to report that; other possibly incorrect situations

Also the following:
https://github.com/phpstan/phpstan-strict-rules

• Require booleans in if, elseif, ternary operator, after !, and on both sides of && and ||.
• Require numeric operands or arrays in + and numeric operands in -/*///**/%.
• Require numeric operand in $var++, $var--, ++$varand --$var.
• These functions contain a $strict parameter for better type safety, it must be set to true:
  • in_array (3rd parameter)
  • array_search (3rd parameter)
  • array_keys (3rd parameter; only if the 2nd parameter $search_value is provided)
  • base64_decode (2nd parameter)
• Variables assigned in while loop condition and for loop initial assignment cannot be used after the loop.
• Variables set in foreach that's always looped thanks to non-empty arrays cannot be used after the loop.
• Types in switch condition and case value must match. PHP compares them loosely by default and that can lead to unexpected results.
• Check that statically declared methods are called statically.
• Disallow empty() - it's a very loose comparison (see manual), it's recommended to use more strict one.
• Disallow short ternary operator (?:) - implies weak comparison, it's recommended to use null coalesce operator (??) or ternary operator with strict condition.
• Disallow variable variables ($$foo, $this->$method() etc.)
• Disallow overwriting variables with foreach key and value variables
• Always true instanceof, type-checking is_* functions and strict comparisons ===/!==. These checks can be turned off by setting checkAlwaysTrueInstanceof/checkAlwaysTrueCheckTypeFunctionCall/checkAlwaysTrueStrictComparison to false.
• Correct case for referenced and called function names.
• Correct case for inherited and implemented method names.
• Contravariance for parameter types and covariance for return types in inherited methods (also known as Liskov substitution principle - LSP)
• Check LSP even for static methods
• Require calling parent constructor
• Disallow usage of backtick operator ($ls = `ls -la`)
• Closure should use $this directly instead of using $this variable indirectly

Fruthermore:

• Enforce the use of \Safe\functions when available.

[codecov]

Codecov Report

Merging #1336 (b7714df) into master (01541c3) will decrease coverage by 0.65%.
The diff coverage is 61.53%.

❗ Current head b7714df differs from pull request most recent head b770145. Consider uploading reports for the commit b770145 to get more accurate results

[d7415]

Looks good to me at a glance. I'd be tempted to leave it enabled so that we see any progress, but I can see how that would look ugly in PRs, especially for externals who aren't expecting red icons!

[qwerty287]

Il agree, but I would enable it in CI, even for PRs. I mean, we should mention in the docs that you should run the make phpstan before submitting the PR, then the contributors can make sure that the code looks good, or we can provide simple help with suggestions or fix issues ourselves.

[d7415]

I agree, but I would enable it in CI, even for PRs. I mean, we should mention in the docs that you should run the make phpstan before submitting the PR, then the contributors can make sure that the code looks good, or we can provide simple help with suggestions or fix issues ourselves.

The problem with this is that until it looks good for Lychee itself, a lot of that will show up for the person doing the PR. unless phpstan can accept a diff somehow.

[d7415]

When I approved this, tests were passing ;)

[qwerty287]

You're right. We need to fix all the issues phpstan reports, then we can enable it.

[ildyria]

You're right. We need to fix all the issues phpstan reports, then we can enable it.

what do you think I am working on ? 😆

I can already tell that @nagmat84 is going to hate me for the conflicts. :')
Which is also why I am doing it files by files per commit.

[nagmat84]

I don't know enough about PhpStan, but I would assume that you can enable/disable/configure individual checks. I haven't had a look at the PR yet, I am only following the discussion. But before you start and take the effort to change a lot of files, we should maybe review the rule set and discuss which rules we want and which not? (Unless PhpStan does not allow configuration.)

[d7415]

Yeah, looks like you can disable checks, files or checks in certain files. @ildyria's already done some of that.

[ildyria]

Yeah, looks like you can disable checks, files or checks in certain files. @ildyria's already done some of that.

Yeah I didn't feel like changing some of the files. :)

[ildyria]

See first post.

[ildyria]

I am considering nuking the empty() rule though...

[nagmat84]

I replaced all, but one empty(...) by more explicit comparisons.

Overall, I believe it was worth the effort, because I actually discovered some bugs. For example, when we convert an EXIF date to a \DateTime, we first checked if the value was empty() and returned null instead of a \DateTime. While this is correct for null and the empty string '', the check would also have returned null if the EXIF date had been zero 0. But 0 is a valid epoch timestamp for 1970-01-01 00:00:00.00000. Probably nobody would have ever discovered that bug. But it is done now.

I left one empty() for @ildyria to solve. That is why the tests are currently failing.

$extension = pathinfo($filename, PATHINFO_EXTENSION);
// Special cases
// https://github.com/electerious/Lychee/issues/482
list($extension) = explode(':', $extension, 2);

if (!empty($extension)) {
	$extension = '.' . $extension;
}

return $extension;

This code is too wicked for me. The problem is not the empty() as such, but I wonder what this code is supposed to do at all or whether it is even correct. explode returns an array with at most 2 entries. But due to list(...) we only pick the 1st entry. Why don't we use explode(':', $extension, 1) in the first place?! And why are we using a colon ':' as separator?!

[ildyria]

This code is too wicked for me. The problem is not the empty() as such, but I wonder what this code is supposed to do at all or whether it is even correct. explode returns an array with at most 2 entries. But due to list(...) we only pick the 1st entry. Why don't we use explode(':', $extension, 1) in the first place?! And why are we using a colon ':' as separator?!

Given the comment, I am pretty sure I didn't touch that code and it is from Electerious (another fellow german BTW).

[nagmat84]

This code is too wicked for me. The problem is not the empty() as such, but I wonder what this code is supposed to do at all or whether it is even correct. explode returns an array with at most 2 entries. But due to list(...) we only pick the 1st entry. Why don't we use explode(':', $extension, 1) in the first place?! And why are we using a colon ':' as separator?!

Given the comment, I am pretty sure I didn't touch that code and it is from Electerious (another fellow german BTW).

Yes, it is. It also refers to an issue from Electerious which is now a dead link. Luckily, this method is gone after we have merged my PR for file streams. So this is only a temporary problem.

[ildyria]

Unless I am wrong, this should finally be ready for merge. 😅

[nagmat84]

I am not entirely sure with 100% certainty, because I have lost track of all the remarks which I have written and whether all of them have actually been addressed. However, I am fine with merging it now and if anything pops up, we have to deal with it in a follow-up PR.

PS: IMHO, Github is really bad at handling big PRs with a long history of comments. A clever strategy would fold all comments with resolved discussion and keep the unresolved discussion unfolded. But Github only seems to apply a static pattern. This means, show the first n comments, hide the next n comments, show n comments again, etc., irrespective of whether those comments are all resolved or still contain an unresolved remark. :-(

[nagmat84]

Approval out of despair 😀 See #1336 (comment)

[kamil4]

PS: IMHO, Github is really bad at handling big PRs with a long history of comments. A clever strategy would fold all comments with resolved discussion and keep the unresolved discussion unfolded. But Github only seems to apply a static pattern. This means, show the first n comments, hide the next n comments, show n comments again, etc., irrespective of whether those comments are all resolved or still contain an unresolved remark. :-(

Yeah, the number of times I needed to click to get to the "New changes since you last viewed" link just now was mind-boggling...

[kamil4]

I left a few non-critical comments.

I feel that given the size of this PR, we should each test it on our devel instances before it gets merged. Has any of you done it? I did not (because I would prefer to wait until we are satisfied with it).

[kamil4]

Places like this make me wonder if we really need to test for both null and empty string everywhere, but I realize that it may not be worth it to figure it out in every single spot...

[nagmat84]

I am convinced that a comparison for null should suffice, because we convert all empty strings to null anyway in the middleware. But I deliberately did it this way, because I thought that someone might ask the exact opposite question if I had done it the other way around.